Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Batched delegations #1201

Merged
merged 8 commits into from
Jun 13, 2024
Merged

Batched delegations #1201

merged 8 commits into from
Jun 13, 2024

Conversation

sampocs
Copy link
Collaborator

@sampocs sampocs commented May 24, 2024

Context

In order to support validator sets of 100+, we have to batch delegations so that we don't exceed the gas limit. This PR handles the changes in the delegation submission. The callback will be in a separate PR.

Note to reviewer: I'd recommend reviewing commit by commit

Brief Changelog

  • Added DelegationTxsInProgress field to deposit record for retry protection - it's incremented when delegation is submitted and will be decremented in the callback
  • Split DelegateOnHost into two functions: GetDelegationICAMessages and BatchSubmitDelegationICAs. The former builds up all the batches across validators, and the latter submits the ICAs in batches
  • Removed the max cap on deposit records - it's unnecessary
  • Added unit tests

@sampocs sampocs changed the title Batch delegations Batched delegations May 24, 2024
Copy link
Contributor

@ethan-stride ethan-stride left a comment

Choose a reason for hiding this comment

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

LGTM very thorough unit tests

This is a summary of the ways in which this one was slightly different from the undelegate batch logic just to confirm my understanding. Deposit records are more temporary than unbonding records because depositing can happen instantly and as soon as it finished we can discard the record, but unbonding needs to know how to accurately claim 21 days+ later. So there is no need for a retry specific queue -- if it fails just put it back into the original queue of ones needing to be handled because all the matters is has it been processed yet or not and once the full amount on the record has been deposited delete the record.

In undelegate batching, you computed the total amount needed to come from a validator so that only one message had to go to each validator affected. But here for deposit the messages are built based on an incoming param of the specific depositRecord. Any deposit it likely to cause msgs to go out to almost all of the validators on its hostZone because of the proportional weighted split strategy. Seems like we actually should aggregate before batching so msgs going to the same validator would be combined. Two depositRecords from the same hostZone seems like a semi common case we can easily reduce the number of ICAs by aggregating per validator, and then chopping the msgs into batches. If you do this, you would need to change the split delegation structure to have different amounts per depositRecord to update the deposit records correctly but it would still be one splitDelegation per msg.

Overall I think it makes sense and is sound as is but we might be able to save many ICAs based on the batching strategy

x/stakeibc/keeper/delegation.go Show resolved Hide resolved
x/stakeibc/keeper/delegation.go Outdated Show resolved Hide resolved
x/stakeibc/keeper/delegation.go Outdated Show resolved Hide resolved
x/stakeibc/keeper/delegation.go Outdated Show resolved Hide resolved
x/stakeibc/keeper/delegation.go Outdated Show resolved Hide resolved
x/stakeibc/keeper/delegation.go Outdated Show resolved Hide resolved
x/stakeibc/keeper/delegation.go Show resolved Hide resolved
x/stakeibc/keeper/delegation_test.go Show resolved Hide resolved
x/stakeibc/keeper/icacallbacks_delegate.go Show resolved Hide resolved
@sampocs
Copy link
Collaborator Author

sampocs commented May 29, 2024

@ethan-stride re: combining deposit records

I think during the design phase we landed on sending messages per one record for simplicity (.e.g. no loops) - I don't think submitting an extra ICA to each validator is too concerning.

Long term, what I think we should really do is combine the two deposit records into 1 (e.g. instead of creating a new record from rewards, we just add the amount to the existing record). And then we can keep the code as is, but it essentially accomplishes the same goal your after. But I think this is outta scope at this point

@sampocs sampocs changed the base branch from main to v23 June 13, 2024 02:04
@sampocs sampocs merged commit 10a1abd into v23 Jun 13, 2024
10 checks passed
Copy link
Contributor

@riley-stride riley-stride left a comment

Choose a reason for hiding this comment

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

Lgtm

Copy link
Contributor

@shellvish shellvish left a comment

Choose a reason for hiding this comment

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

Looks great, thanks a ton for doing this!

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

Successfully merging this pull request may close these issues.

4 participants