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

feat: sbtc limits handling in signer + emily integration #811

Merged
merged 25 commits into from
Dec 1, 2024

Conversation

cylewitruk
Copy link
Member

@cylewitruk cylewitruk commented Nov 11, 2024

Description

Addresses sBTC caps described in #854 & #856.

Changes

  • Adds the Emily integration for getting limits
  • Blocks signer-startup until the most recent limits have been fetched. This is to ensure that the signer has up-to-date values upon startup.
  • Adds a new loop which checks/updates limits every 5 minutes thereafter -- we could probably do this as part of the block-observer trigger instead if we wanted to. There was a reason why I didn't but I don't remember 🙈
  • Filters both deposits and withdrawals based on the per-request cap. This is handled as part of the decision process which allows the request to be "handled" while also reporting that the signer won't sign for it.

Other Info

PR #904 added support for getting the current circulating sBTC amount from the sbtc-token contract, however I'm not entirely sure how we want to implement total-peg caps.

For instance, if the cap is 100 and total-circulating is currently 95, if we get 7 deposit requests of 1 BTC each then that would theoretically put us at 102. Putting the total cap filter on the decision level would mean we only accept 5 of those, but we don't know if those will manage to be signed and swept at this point (another signer might say "no").

What might feel more correct is having the transaction coordinator's packaging only construct packages that keep us under the total cap, and then have each signer validate that against the total-circulating amount that their own Stacks node reports during the signing-validation process.

@cylewitruk cylewitruk self-assigned this Nov 11, 2024
@cylewitruk cylewitruk added this to the sBTC: Deposit ready milestone Nov 11, 2024
@cylewitruk cylewitruk marked this pull request as draft November 20, 2024 14:34
docker/docker-compose.yml Outdated Show resolved Hide resolved
@cylewitruk cylewitruk changed the title feat: suggestion on sbtc limits handling in signer feat: sbtc limits handling in signer + emily integration Nov 20, 2024
Copy link
Collaborator

@Jiloc Jiloc left a comment

Choose a reason for hiding this comment

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

Just a few sanity checks. My main question is why this is handled in a separate sidecar that updates every 5 minutes, rather than during the creation and processing of new transactions. That said, the functionality remains unchanged, so I’m completely okay with it. The only potential edge case I see is if we lowered the cap while signers were validating new transactions, but that’s highly unlikely, and we’re not planning anything like that. Overall, I’m fine with the current implementation!

docker/docker-compose.yml Show resolved Hide resolved
signer/src/emily_client.rs Show resolved Hide resolved
signer/src/request_decider.rs Show resolved Hide resolved
signer/src/request_decider.rs Show resolved Hide resolved
@cylewitruk
Copy link
Member Author

cylewitruk commented Nov 25, 2024

Just a few sanity checks. My main question is why this is handled in a separate sidecar that updates every 5 minutes, rather than during the creation and processing of new transactions. That said, the functionality remains unchanged, so I’m completely okay with it. The only potential edge case I see is if we lowered the cap while signers were validating new transactions, but that’s highly unlikely, and we’re not planning anything like that. Overall, I’m fine with the current implementation!

So my thinking was largely that this value should almost never change and to avoid extra calls to Emily that maybe this would be once an hour or something instead of every block (or 5m as it's coded right now). I also wanted to avoid piling additional things on-top of the block-observer checks in general. I don't have a strong opinion either way though tbh.

@Jiloc
Copy link
Collaborator

Jiloc commented Nov 26, 2024

That makes sense. The more I think about it, the only way to make this check synchronous with the deposit request would be to have it act an emergency stop mechanism to halt deposits immediately if something unusual happens. However, this isn’t meant to serve that purpose, so I agree that we’re fine as it is. Keeping the block-observer "lean" is definitely a good goal.

@cylewitruk
Copy link
Member Author

Note that this PR is still missing tests, hence the draft status. I'm going to work on those next.

@aldur aldur assigned Jiloc and unassigned cylewitruk Nov 29, 2024
@aldur aldur marked this pull request as ready for review November 29, 2024 16:22
Jiloc and others added 3 commits November 29, 2024 18:10
* cap deposits when constructing new transactions

* move cap control in the validate method. Sync check for caps

* add total amount check

* clean up

* add tests

* fix error

* address review comments

* centralize limits update in block observer

* fix error message

Co-authored-by: Matteo Almanza <[email protected]>

---------

Co-authored-by: Matteo Almanza <[email protected]>
signer/src/bitcoin/validation.rs Outdated Show resolved Hide resolved
signer/src/context/signer_state.rs Outdated Show resolved Hide resolved
signer/src/block_observer.rs Show resolved Hide resolved
signer/src/block_observer.rs Show resolved Hide resolved
Copy link
Collaborator

@matteojug matteojug left a comment

Choose a reason for hiding this comment

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

mostly nits, LGTM

signer/src/bitcoin/validation.rs Outdated Show resolved Hide resolved
signer/src/bitcoin/validation.rs Outdated Show resolved Hide resolved
signer/src/block_observer.rs Show resolved Hide resolved
signer/src/block_observer.rs Outdated Show resolved Hide resolved
signer/src/context/signer_state.rs Show resolved Hide resolved
signer/tests/integration/emily.rs Outdated Show resolved Hide resolved
@Jiloc Jiloc merged commit b3eb827 into main Dec 1, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants