STR-734: OPERATOR REASSIGNMENT #557

voidash · 2024-12-23T07:27:07Z

Description

Functional test where we make deposits, then trigger the withdrawal. client associated with assigned operator id is stopped. We produce l1 blocks so that the dispatch assignment duration is over, and later check if new operator is being assigned or not

Type of Change

Bug fix (non-breaking change which fixes an issue)
New feature/Enhancement (non-breaking change which adds functionality or enhances an existing one)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Documentation update
Refactor
New or updated tests
Dependency Update

Notes to Reviewers

Checklist

I have performed a self-review of my code.
I have commented my code where necessary.
I have updated the documentation if needed.
My changes do not introduce new warnings.
I have added tests that prove my changes are effective or that my feature works.
New and existing tests pass with my changes.

Related Issues

STR-734

codecov · 2024-12-23T07:32:54Z

Codecov Report

Attention: Patch coverage is 0% with 32 lines in your changes missing coverage. Please review.

Project coverage is 57.27%. Comparing base (506fd5c) to head (613ba96).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...n/bridge-client/src/modes/operator/task_manager.rs	0.00%	18 Missing ⚠️
bin/bridge-client/src/modes/operator/bootstrap.rs	0.00%	8 Missing ⚠️
crates/bridge-exec/src/handler.rs	0.00%	3 Missing ⚠️
crates/chaintsn/src/transition.rs	0.00%	3 Missing ⚠️

@@            Coverage Diff             @@
##             main     #557      +/-   ##
==========================================
- Coverage   57.33%   57.27%   -0.06%     
==========================================
  Files         309      309              
  Lines       31510    31520      +10     
==========================================
- Hits        18067    18054      -13     
- Misses      13443    13466      +23

Files with missing lines	Coverage Δ
bin/bridge-client/src/args.rs	`0.00% <ø> (ø)`
crates/bridge-exec/src/handler.rs	`0.00% <0.00%> (ø)`
crates/chaintsn/src/transition.rs	`82.86% <0.00%> (ø)`
bin/bridge-client/src/modes/operator/bootstrap.rs	`0.00% <0.00%> (ø)`
...n/bridge-client/src/modes/operator/task_manager.rs	`0.00% <0.00%> (ø)`

... and 3 files with indirect coverage changes

storopoli

Maybe we should create a proper parent class for tests that needs withdrawal/deposits that can have all the yet-to-move utils.py functions that I commented below in the NITs.

Then all the DRT, get balances, withdraw etc functionality is encapsulated in it and we can create proper concrete classes for the other tests that inherit from this parent class.

Sorry to push all this refactoring into your plate.
Since you're already editing these files it is convenient.
However, feel free to reject and open a ticket for this so that we can do it later.

functional-tests/fn_bridge_reassignment.py

storopoli · 2024-12-23T11:53:00Z

functional-tests/fn_bridge_reassignment.py

+from fn_bridge_withdraw_happy import (
+    DEPOSIT_AMOUNT,
+    check_initial_eth_balance,
+    deposit_twice,
+    do_withdrawal,
+    setup_services,
+)


Maybe we should move these to utils?
What you think?
They are being used elsewhere than fn_bridge_withdraw_happy.

storopoli · 2024-12-23T11:53:45Z

functional-tests/fn_bridge_reassignment.py

+        # Check initial balance is 0
+        check_initial_eth_balance(rethrpc, el_address, self.debug)
+
+        # Perform two deposits
+        deposit_twice(ctx, web3, seqrpc, rethrpc, el_address, self.debug)


I think that there's a way for you to setup an environment with already balances in BTC and Strata, AFAIK @evgenyzdanovich implemented that feature.

This aligns with the "minimal resources" principle above.

storopoli · 2024-12-23T11:55:07Z

functional-tests/fn_bridge_reassignment.py

+
+    def __init__(self, ctx: flexitest.InitContext):
+        # Example: we spin up 5 operators
+        ctx.set_env(testenv.BasicEnvConfig(101, n_operators=5))


I think that you can also disable auto-generate BTC blocks. This is important to avoid flaky tests when you need total control of the block production (i.e. waiting for 64 blocks below).

functional-tests/fn_bridge_withdraw_happy.py

storopoli · 2024-12-23T11:57:42Z

functional-tests/fn_bridge_reassignment.py

+        # Re-check duties
+        duties = seqrpc.strata_getBridgeDuties(0, 0)["duties"]
+        withdraw_duty = [d for d in duties if d["type"] == "FulfillWithdrawal"][0]
+        new_assigned_op_idx = withdraw_duty["payload"]["assigned_operator_idx"]
+        new_assigned_operator = ctx.get_service(f"bridge.{new_assigned_op_idx}")
+
+        # Ensure a new operator is assigned
+        assert new_assigned_operator != assigned_operator, "No new operator was assigned"
+
+        return True


We are testing only 2 out of 3 things that are highlighted in the original ticket (STR-734):

~~Is the user’s withdrawal request confirmed in a Strata batch?~~

~~After the first operator fails to initiate the withdrawal, is the withdrawal request properly assigned to another operator?~~

After the withdrawal request is assigned to another operator, is it signed by all other operators, broadcast, and confirmed on signet?

We need to find a way to test number 3 above.

Yeah I guess once we observe that it's been reassigned we can restart the operator we shut down and then get it signed.

delbonis · 2024-12-23T18:48:47Z

I think it's reasonable to include the fix for this ticket in this PR too, since it's all related.

functional-tests/fn_bridge_reassignment.py

bewakes · 2024-12-24T04:31:09Z

functional-tests/fn_bridge_withdraw_happy.py

+    """
+    # Get bridge pubkey
+    bridge_pk = utils.get_bridge_pubkey(seqrpc)
+    if debug_fn:


Can we not just omit this if clause? Its annoying to use this here and there. The default value in the debug_fn argument indicates that this clause will always return true, unless None is passed(but this can be avoided by enforcing argument type).

Agreed! The way to enable debug messages like these should be to set the LOG_LEVEL and not pass in an explicit argument at runtime.

These are test specific log, so this is the easiest way to inject logs or we have to pass test name to getLogger

In that case, I second the approach by @bewakes i.e., annotating with Callable[str, None] perhaps and removing the if clause.

Rajil1213 · 2024-12-24T11:08:16Z

bin/bridge-client/src/modes/operator/task_manager.rs


            // if none of the duties failed, update the duty index so that the
            // next batch is fetched in the next poll.
            //
            // otherwise, don't update the index so that the current batch is refetched and
            // ones that were not executed successfully are executed again.


Need to move these comments on top of the if !any_failed check as well.

Rajil1213 · 2024-12-24T11:08:41Z

crates/bridge-exec/src/handler.rs

@@ -297,6 +297,9 @@ where
                if let Ok(msg) = msg {
                    let payload = msg.payload();
                    let parsed_payload = borsh::from_slice::<Payload>(payload);
+                    let scope = msg.scope();
+                    let parsed_scope = borsh::from_slice::<Scope>(scope);
+                    debug!(msg=?msg, payload=?parsed_payload, scope=?parsed_scope, "ABC: got message from the L2 Client");


Suggested change

debug!(msg=?msg, payload=?parsed_payload, scope=?parsed_scope, "ABC: got message from the L2 Client");

debug!(msg=?msg, payload=?parsed_payload, scope=?parsed_scope, "got message from the L2 Client");

Also fix the formatting here. If the name of the log field is the same as the variable being used for it, you can just omit the field name and make it ?msg (or %msg for Display print). In any case, there should be spaces around the =. Rustfmt can't understand the macro conventions so it can't automatically fix this.

Rajil1213 · 2024-12-24T11:14:54Z

functional-tests/fn_bridge_reassignment.py

+        # Re-check duties
+        def my_val():
+            btcrpc.proxy.generatetoaddress(64, UNSPENDABLE_ADDRESS)
+            duties = seqrpc.strata_getBridgeDuties(0, 0)["duties"]
+            withdraw_duty = [d for d in duties if d["type"] == "FulfillWithdrawal"][0]
+            new_assigned_op_idx = withdraw_duty["payload"]["assigned_operator_idx"]
+            new_assigned_operator = ctx.get_service(f"bridge.{new_assigned_op_idx}")
+            self.debug(f"new assigned operator is {new_assigned_op_idx}")
+            return new_assigned_operator
+
+        new_assigned_operator = wait_until_with_value(
+            my_val,
+            predicate=lambda v: v != assigned_operator,
+            timeout=10,
+        )


I think this check can be removed now since the reassignment logic is supposed to always compute a different assignee than the current one (by computing offset).

Rajil1213 · 2024-12-24T11:16:17Z

functional-tests/fn_bridge_reassignment.py

+        # Ensure a new operator is assigned
+        assert new_assigned_operator != assigned_operator, "No new operator was assigned"
+        assigned_operator.start()
+        abc = assigned_operator.create_rpc()


Suggested change

abc = assigned_operator.create_rpc()

bridge_rpc = assigned_operator.create_rpc()

Rajil1213 · 2024-12-24T11:16:59Z

functional-tests/fn_bridge_reassignment.py

+    """
+
+    def __init__(self, ctx: flexitest.InitContext):
+        # Example: we spin up 5 operators


Suggested change

# Example: we spin up 5 operators

# Example: we spin up 3 operators

Actually, you can just remove this comment as it is very self-evident.

Rajil1213 · 2024-12-24T11:20:43Z

bin/bridge-client/src/constants.rs

@@ -4,5 +4,6 @@ pub(super) const DEFAULT_RPC_PORT: u32 = 4781; // first 4 digits in the sha256 o

 pub(super) const DEFAULT_RPC_HOST: &str = "127.0.0.1";

+pub(super) const DEFAULT_DUTY_TIMEOUT_DURATION: u64 = 65;


Should increase this default value to at least 600. I know this is a temporary measure but this is to avoid breaking stuff if we ever deploy this change to our signet (whether accidentally or not).

Rajil1213 · 2024-12-24T11:22:07Z

crates/chaintsn/src/transition.rs

                    } else {
+                        // If there is 0 or 1 operator, we remain with the current assignee.


Suggested change

// If there is 0 or 1 operator, we remain with the current assignee.

// If there is only a single operator, we remain with the current assignee.

There can't technically be 0 operators or dstate.assignee() wouldn't work.

Yeah I think we refuse to start the client with 0 operators.

Rajil1213 · 2024-12-24T11:22:47Z

functional-tests/fn_bridge_reassignment.py

+    Makes two DRT deposits, then triggers the withdrawal.
+    The bridge client associated with assigned operator id is stopped.
+    After the dispatch assignment duration is over,
+    Check if new operator is being assigned or not


Suggested change

Check if new operator is being assigned or not

Check if new operator is being assigned or not

Ensure that the withdrawal resumes and completes successfully.

voidash added 2 commits December 23, 2024 01:14

func-tests: add bridge reassignment check

96d020e

func-test: add bridge withdraw reassignment

470927d

voidash requested review from a team as code owners December 23, 2024 07:27

storopoli requested changes Dec 23, 2024

View reviewed changes

bridge-client: add timeout for duties

e4b9e44

bewakes reviewed Dec 24, 2024

View reviewed changes

functional-tests/fn_bridge_reassignment.py Show resolved Hide resolved

bewakes reviewed Dec 24, 2024

View reviewed changes

bridge-client: add duty_timeout_duration

f0983f3

voidash requested review from a team as code owners December 24, 2024 10:50

chaintsn: STR-781 fix reassignment bug

613ba96

voidash requested a review from a team as a code owner December 24, 2024 11:05

Rajil1213 requested changes Dec 24, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

STR-734: OPERATOR REASSIGNMENT #557

STR-734: OPERATOR REASSIGNMENT #557

voidash commented Dec 23, 2024

codecov bot commented Dec 23, 2024 •

edited

Loading

storopoli left a comment

storopoli Dec 23, 2024

storopoli Dec 23, 2024

storopoli Dec 23, 2024

storopoli Dec 23, 2024

storopoli Dec 23, 2024

delbonis Dec 23, 2024

delbonis commented Dec 23, 2024

bewakes Dec 24, 2024 •

edited

Loading

Rajil1213 Dec 24, 2024

voidash Dec 24, 2024

Rajil1213 Dec 24, 2024

Rajil1213 Dec 24, 2024

Rajil1213 Dec 24, 2024

delbonis Dec 24, 2024

Rajil1213 Dec 24, 2024

Rajil1213 Dec 24, 2024

Rajil1213 Dec 24, 2024

Rajil1213 Dec 24, 2024 •

edited

Loading

Rajil1213 Dec 24, 2024

delbonis Dec 24, 2024

Rajil1213 Dec 24, 2024

	debug!(msg=?msg, payload=?parsed_payload, scope=?parsed_scope, "ABC: got message from the L2 Client");
	debug!(msg=?msg, payload=?parsed_payload, scope=?parsed_scope, "got message from the L2 Client");

	abc = assigned_operator.create_rpc()
	bridge_rpc = assigned_operator.create_rpc()

	# Example: we spin up 5 operators
	# Example: we spin up 3 operators

		@@ -4,5 +4,6 @@ pub(super) const DEFAULT_RPC_PORT: u32 = 4781; // first 4 digits in the sha256 o

		pub(super) const DEFAULT_RPC_HOST: &str = "127.0.0.1";

		pub(super) const DEFAULT_DUTY_TIMEOUT_DURATION: u64 = 65;

		} else {
		// If there is 0 or 1 operator, we remain with the current assignee.

	// If there is 0 or 1 operator, we remain with the current assignee.
	// If there is only a single operator, we remain with the current assignee.

	Check if new operator is being assigned or not
	Check if new operator is being assigned or not
	Ensure that the withdrawal resumes and completes successfully.

STR-734: OPERATOR REASSIGNMENT #557

Are you sure you want to change the base?

STR-734: OPERATOR REASSIGNMENT #557

Conversation

voidash commented Dec 23, 2024

Description

Type of Change

Notes to Reviewers

Checklist

Related Issues

codecov bot commented Dec 23, 2024 • edited Loading

Codecov Report

storopoli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

delbonis commented Dec 23, 2024

bewakes Dec 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rajil1213 Dec 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Dec 23, 2024 •

edited

Loading

bewakes Dec 24, 2024 •

edited

Loading

Rajil1213 Dec 24, 2024 •

edited

Loading