stacks-network / sbtc-developer-release

sBTC primitives, signer components, helper tools
https://sbtc.tech
MIT License
1.98k stars 23 forks source link

Romeo should pass the unreversed txid to asset contract #164

Closed radicleart closed 1 year ago

radicleart commented 1 year ago

The deposit_txid parameter of this stacks tx is reversal of the actual bitcoin txid

Romeo should pass the unreversed txid so app developers can use this value to lookup bitcoin transactions without worrying about first having to reverse the byte order.

Note the reason for reversing the txid is because the asset contract calls the clarity-bitcoin-mini to confirm mining of the transaction which requires the reversed txid. However clarity-bitcoin-mini has a method to compute the reversed txid.

The comments for method verify-merkle-proof in clarity-bitcoin-mini also need to be updated to make it clear it takes the unreversed txid as input.

Definition of done

Txids in contract function arguments should be shown in the same canonical format as in Bitcoin exporers.

stjepangolemac commented 1 year ago

@friedger what's the default way of representing bitcoin txids on the wire, reversed or not?

friedger commented 1 year ago

@stjepangolemac as the txid is shown in the stacks explorer it should be shown in the same way as in the bitcoin explorer for user to easy understand.

stjepangolemac commented 1 year ago

So if I've understood it correctly the only place where we can see the txid being reversed in comparison to the stacks and bitcoin explorers are deposit/withdrawal-txid fields. Can we reverse it in the contract so that arguments are clearer for an observer?

radicleart commented 1 year ago

The issue is that Romeo sends it reversed which means in the stacks explorer and in clarity events the txid will be reversed - this will confuse a lot of people because they will copy/paste the txid into eg mempool and see non existent transaction. e.g.

Screenshot 2023-09-26 at 13 42 47
radicleart commented 1 year ago

I beleive the Clarity-Bitcoin-Mini : was-txid-mined also reverses the txid - ie reverses the reversed txid !

friedger commented 1 year ago

To clarify, clarity-bitcoin-mini expect the txid in the same style as shown in the explorers, that is in big endian. The txid is reversed where necessary in the contract.

stjepangolemac commented 1 year ago

Okay I've updated the description. I'll take this when the bulk of other Romeo work is done.

friedger commented 1 year ago

In devenv, I see that romeo sends the txid in the correct format.

I have added another test to confirm that in branch 164-txid

Screenshot from 2023-09-28 11-48-20 Screenshot from 2023-09-28 11-48-44

radicleart commented 1 year ago

Can you post the proof data ? Maybe i can run the bridge locally on regtest and check it against the bridge proof data.

stjepangolemac commented 1 year ago

Seems like txids are passed correctly after all? To be honest, I'm a bit confused with all this.

radicleart commented 1 year ago

I've not been able to retest @stjepangolemac because of Hiro rate limit issues running romeo.

The mint here was called by Romeo on testnet in response to this deposit

the deposit-txid printed in the explorer is reverse of the bitcoin txid 0xd33e92fcecc594f70442600b280b5901e06fb84d7d4253bf15d4e1257b46d801

deposit-txid gets passed to was-tx-mined as txid. Not sure whats going on but reproducing the test on testnet will help clarify.

radicleart commented 1 year ago

Retested this and correct big endian txid is being passed to stacks clarity contract.