Update StackStxOp to include signer key
Description
Following the signer-key authorization in #4377, this PR updates stack-stx burn op (StackStxOp) to include signer-key.
Applicable issues
- closes #4285
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 dataPowered by Codecov. Last update 0e9cf5d...cb136bf. Read the comment docs.
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)
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.
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.
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.