stacks-core icon indicating copy to clipboard operation
stacks-core copied to clipboard

Update StackStxOp to include signer key

Open 8marz8 opened this issue 1 year ago • 1 comments

Description

Following the signer-key authorization in #4377, this PR updates stack-stx burn op (StackStxOp) to include signer-key.

Applicable issues

  • closes #4285

8marz8 avatar Feb 22 '24 13:02 8marz8

Codecov Report

Attention: Patch coverage is 95.51167% with 50 lines in your changes are missing coverage. Please review.

Project coverage is 83.39%. Comparing base (0e9cf5d) to head (cb136bf). Report is 7 commits behind head on next.

Additional details and impacted files
@@            Coverage Diff             @@
##             next    #4412      +/-   ##
==========================================
+ Coverage   83.33%   83.39%   +0.06%     
==========================================
  Files         453      453              
  Lines      328494   329585    +1091     
  Branches      323      323              
==========================================
+ Hits       273744   274853    +1109     
+ Misses      54742    54724      -18     
  Partials        8        8              
Files Coverage Δ
...c/chainstate/burn/operations/test/serialization.rs 100.00% <100.00%> (ø)
stackslib/src/chainstate/coordinator/tests.rs 90.80% <100.00%> (+0.01%) :arrow_up:
stackslib/src/chainstate/burn/operations/mod.rs 62.36% <75.00%> (+0.54%) :arrow_up:
stackslib/src/chainstate/burn/db/sortdb.rs 91.97% <91.66%> (+<0.01%) :arrow_up:
stackslib/src/chainstate/stacks/db/blocks.rs 90.04% <92.85%> (+<0.01%) :arrow_up:
...ckslib/src/chainstate/burn/operations/stack_stx.rs 86.67% <94.37%> (+3.61%) :arrow_up:
...net/stacks-node/src/tests/nakamoto_integrations.rs 95.56% <97.12%> (+0.29%) :arrow_up:
testnet/stacks-node/src/tests/neon_integrations.rs 74.12% <97.01%> (+0.82%) :arrow_up:
...-node/src/burnchains/bitcoin_regtest_controller.rs 87.30% <80.88%> (+0.12%) :arrow_up:

... and 22 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 0e9cf5d...cb136bf. Read the comment docs.

codecov[bot] avatar Feb 22 '24 21:02 codecov[bot]

Ah, I just realized that https://github.com/stacks-network/stacks-core/pull/4461 will require an update to this burn op (two new u128 fields)

hstove avatar Mar 04 '24 22:03 hstove

So I pushed a small change to fix a test timeout issue, but now I'm seeing an issue where the BTC op isn't being "accepted" by the regtest controller. There isn't much of an error, but I think it might be that the total size of the OP_RETURN is too large. We already squeezed auth_id down to u64, and I'm not sure if downsizing max_amount is a good idea.

hstove avatar Mar 12 '24 17:03 hstove

It took me a while to figure out why the integration test was failing, but I finally narrowed it down to not having enough balance when building the stack-stx operation transaction.

I also modified the auth_id data to be u32, because it's not totally clear to me if using u64 would put us over the OP_RETURN max length when including all the other overhead. If this is not the right call, we can just revert my last commit.

hstove avatar Mar 14 '24 23:03 hstove

As I was updating this PR for one of @jcnelson's review comments, I noticed a logic bug: the function process_stacking_ops automatically calls set-signer-key-authorization to ensure that the internal Clarity transaction is successful. We missed this on previous reviews, but we definitely don't want this behavior: the whole point (for the most part) of set-signer-key-authorization is to support having stack-stx burn ops without needing the whole signature. However, we still want to ensure that the signer has provided this authorization beforehand. With the way this PR currently works, you could use this burn op to bypass the whole signer key authorization check.

This isn't a huge change - we'll need to strip out the mentioned code in process_stacking_ops, and then update the integration tests to add the set-signer-key-authorization check before making the burn ops.

hstove avatar Mar 19 '24 13:03 hstove