feat: check against known burn hash when completing deposit #493
Conversation
| (current-signer-data (contract-call? .sbtc-registry get-current-signer-data)) | ||
| ) | ||
|
|
||
| (define-public (complete-deposits-wrapper |
There was a problem hiding this comment.
We need to update the complete-deposit-wrapper function too.
| clarity_version = 3 | ||
| epoch = 3.0 |
There was a problem hiding this comment.
Oh interesting, I didn't realize that clarity v3 was a thing. I didn't see any mention of it in the docs, where can I find out more about this? (Well, I've only looked here https://docs.stacks.co/reference/functions for info in clarity v3)
There was a problem hiding this comment.
Yeah it's pretty much just an update to include some nakamoto-specific functions like for tenures etc. Don't think the docs are published yet but they will be generated from here:
https://github.com/stacks-network/stacks-core/tree/develop/clarity%2Fsrc%2Fvm%2Fdocs
| ;; (get-burn-block-info? header-hash height) | ||
| (get-tenure-info? burnchain-header-hash height) |
There was a problem hiding this comment.
Oh interesting, I don't know much about get-tenure-info?. I was able to track it down in the source but my main question is whether it returns the exact same info as get-burn-block-info?. I suppose it probably does, but I couldn't follow the source much to verify.
|
Should we update the |
|
Reviewing this, just want to make sure I'm making sense here: tests aren't done / might not be possible for the burn hash check since Clarinet is generating pseudo-random info in 'get-burn-block-info?. |
Yeah I think that is my understanding too. Well, I mean this:
|
| (define-public (complete-withdrawals (withdrawals (list 600 | ||
| {request-id: uint, | ||
| status: bool, | ||
| signer-bitmap: uint, | ||
| bitcoin-txid: (optional (buff 32)), | ||
| output-index: (optional uint), | ||
| fee: (optional uint)}))) | ||
| {request-id: uint, | ||
| status: bool, | ||
| signer-bitmap: uint, | ||
| bitcoin-txid: (optional (buff 32)), | ||
| output-index: (optional uint), | ||
| fee: (optional uint)})) | ||
| (burn-height uint) | ||
| (burn-hash (buff 32))) |
There was a problem hiding this comment.
Can we do this for accept-withdrawal-request as well?
There was a problem hiding this comment.
Completed tests & was just looking at this & it requires a bit more refactor that I thought going in. As a result of using a helper to loop through multiple deposits & withdrawals I see two options moving forward, neither of which are great:
- We update individual deposits & withdrawals with a burn-height & burn-hash field (which I don't love since multiple deposits & withdrawals might now have different burn-heights & burn-hashes)
- We don't include burn-height/burn-hash in individual deposits & withdrawals but instead print them both from the sbtc-deposit & sbtc-withdrawal contracts instead of in the confirmed sbtc-registry contract (which I don't love since we currently have all print statements in sbtc-registry)
There was a problem hiding this comment.
Yeah, I definitely prefer option (1). And there is also option (3): each withdrawal tuple has a burn-height and burn-hash field. The trade off there is that we need to submit more data when making the contract call, making things more expensive. This is a slight modification on option (1) but I like (1) more.
But I think we can keep the updated signature as is and clarity things in the docs: only make complete-withdrawals and complete-deposits contract calls where the requests were completed in the same bitcoin block.
setzeus
force-pushed
the
feat/check-burn-hash
branch
from
f9ebf72
to
7f28d91
Compare
|
I just pushed some fixes for the failing CI test. It was two things:
|
This PR updates the
complete-deposits-wrapperfunction to take aburn-heightandburn-hash. Then the contract calls(get-tenure-info? burnchain-header-hash)to verify that the "known" burn hash at that height matches the argument provided. This allows deposits to be processed right away, while still defending against Bitcoin forks. If Bitcoin forks, these deposit transactions will need to be re-submitted with the new (height, hash) tuple.I had trouble using
get-burn-block-info?with Clarinet - I think Clarinet mocks that function and returns pseudo-random information, so I couldn't get a matching hash to work. Usingget-tenure-info?worked as expected. This required updating our contracts to use Clarity version 3. I also had to push an update to Clarigen to get things working correctly with Clarity v3.