Skip to content

Add explicit presignature index assignment#331

Open
zhouwfang wants to merge 3 commits intomainfrom
zhoufang/iop-285-add-explicit-presignature-index-assignment
Open

Add explicit presignature index assignment#331
zhouwfang wants to merge 3 commits intomainfrom
zhoufang/iop-285-add-explicit-presignature-index-assignment

Conversation

@zhouwfang
Copy link
Contributor

@zhouwfang zhouwfang requested a review from bmwill as a code owner March 19, 2026 19:21
@zhouwfang zhouwfang force-pushed the zhoufang/iop-285-add-explicit-presignature-index-assignment branch 2 times, most recently from 2da98ff to 58ea608 Compare March 19, 2026 19:30
@zhouwfang zhouwfang force-pushed the zhoufang/iop-285-add-explicit-presignature-index-assignment branch from 58ea608 to 77013e3 Compare March 19, 2026 19:31
@zhouwfang zhouwfang force-pushed the zhoufang/iop-285-add-explicit-presignature-index-assignment branch 4 times, most recently from 1e51e3b to 1484d9e Compare March 19, 2026 21:53
@zhouwfang zhouwfang force-pushed the zhoufang/iop-285-add-explicit-presignature-index-assignment branch 4 times, most recently from cdfba92 to 7457c2f Compare March 19, 2026 22:08
@zhouwfang zhouwfang requested review from Bridgerz and bmwill March 19, 2026 22:13
@Bridgerz
Copy link
Contributor

Bridgerz commented Mar 19, 2026

Looking good! Can we also get tests for:

  • Move test for reassign_presig_indices (epoch transition reassignment)
  • Rust test for epoch mismatch rejection in mpc_sign_withdrawal_tx
  • Rust test for the multi-batch-jump edge case

@zhouwfang zhouwfang force-pushed the zhoufang/iop-285-add-explicit-presignature-index-assignment branch from 7457c2f to 7dd5024 Compare March 19, 2026 22:50
@zhouwfang zhouwfang requested a review from Bridgerz March 19, 2026 22:55
requests: Bag,
pending_withdrawals: Bag,
/// Tracks pending withdrawal IDs for iteration (Bag doesn't support iteration).
pending_withdrawal_ids: vector<address>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may warant changing the pending_withdrawal_ids data structure to something like a LinkedTable.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So i'm not a huge fan of this because it requires unbounded iteration that must terminate inside of a single sui transaction. These types of things can lead to deadlock situations where a txn can never terminate because it takes too much gas or loads too many objects. imo i don't think we need this, or the ability to reassign indexes in the end_reconfig function and instead should introduce another function that can be used to re-key an explicitly provided set of ids. Given the epoch is encoded in there we can iterate through these offchain and selectively bump them before we go to signing them.

Thoughts? @manolisliolios can keep me honest here too

Copy link
Contributor

@Bridgerz Bridgerz Mar 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. puts a lot of load on the end of epoch tx as well as possible deadlock on that critical tx. I think a new function for reassigning explicit withdrawals makes the most sense. We will need to guard this new function with a certificate like we do for each stage of the withdrawal flow (ex. confirm_withdrawal)

Copy link
Contributor Author

@zhouwfang zhouwfang Mar 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My original approach was lazy reassignment at signing time. I turned to this approach because the original approach requires a fair amount of plumbing (new RPC endpoint, and leader logic to fan out, aggregate, submit), and there was some urgency to unblock devnet. But it may be no longer urgent to merge this PR now as the issue it tries to fix is very unlikely to happen (see my elaboration in chat), and I can turn back to the original approach. WDYT?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants