refactor: use bosd::Descriptor for operator reimbursement

[sistemd]

Description

Change the operator reimbursement address type to bosd::Descriptor. Because some of the descriptor operations are fallible, there is a new error that has to be propagated throughout some parts of the call stack.

Type of Change

• Bug fix (non-breaking change which fixes an issue)
• New feature/Enhancement (non-breaking change which adds functionality or enhances an existing one)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Documentation update
• Refactor
• New or updated tests
• Dependency Update

Checklist

• I have performed a self-review of my code.
• I have commented my code where necessary.
• I have updated the documentation if needed.
• My changes do not introduce new warnings.
• I have added (where necessary) tests that prove my changes are effective or that my feature works.
• New and existing tests pass with my changes.

Related Issues

Closes STR-1150.

[codecov]

Codecov Report

❌ Patch coverage is 44.76190% with 174 lines in your changes missing coverage. Please review.
✅ Project coverage is 51.44%. Comparing base (3f8bebc) to head (105a45f).

Files with missing lines	Patch %	Lines
crates/duty-tracker/src/contract_state_machine.rs	0.00%	46 Missing ⚠️
crates/duty-tracker/src/contract_manager.rs	0.00%	43 Missing ⚠️
...ates/duty-tracker/src/stake_chain_state_machine.rs	0.00%	29 Missing ⚠️
bin/alpen-bridge/src/rpc_server.rs	0.00%	19 Missing ⚠️
crates/stake-chain/src/stake_chain.rs	56.25%	7 Missing ⚠️
...duty-tracker/src/executors/contested_withdrawal.rs	0.00%	6 Missing ⚠️
crates/tx-graph/src/peg_out_graph.rs	89.83%	6 Missing ⚠️
crates/stake-chain/src/transactions/stake.rs	72.22%	5 Missing ⚠️
bin/dev-cli/src/handlers/disprove.rs	0.00%	4 Missing ⚠️
...uty-tracker/src/executors/optimistic_withdrawal.rs	0.00%	4 Missing ⚠️
... and 2 more

@@            Coverage Diff             @@
##             main     #286      +/-   ##
==========================================
- Coverage   51.51%   51.44%   -0.08%     
==========================================
  Files         146      146              
  Lines       21935    22015      +80     
==========================================
+ Hits        11300    11325      +25     
- Misses      10635    10690      +55     

Files with missing lines	Coverage Δ
crates/connectors/src/connector_s.rs	99.15% <ø> (ø)
crates/primitives/src/types.rs	70.00% <100.00%> (+70.00%)	⬆️
crates/tx-graph/src/transactions/assert_chain.rs	81.08% <100.00%> (+0.17%)	⬆️
crates/tx-graph/src/transactions/assert_data.rs	92.61% <100.00%> (+0.02%)	⬆️
crates/tx-graph/src/transactions/challenge.rs	86.09% <100.00%> (+0.18%)	⬆️
crates/tx-graph/src/transactions/claim.rs	91.02% <100.00%> (ø)
crates/tx-graph/src/transactions/cpfp.rs	98.27% <100.00%> (+<0.01%)	⬆️
crates/tx-graph/src/transactions/payout.rs	97.20% <100.00%> (-0.06%)	⬇️
...tes/tx-graph/src/transactions/payout_optimistic.rs	95.73% <100.00%> (-0.16%)	⬇️
crates/tx-graph/src/transactions/pre_assert.rs	95.71% <100.00%> (ø)
... and 13 more

... and 2 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Rajil1213]

Good work, but I do have a couple of comments:

1. We (in the bridge) prefer commits to be atomic. Right now, this PR changes 28 files with a single commit which makes it very difficult to review (since there is no line of reasoning to follow which would be provided by a proper commit history).
2. The type of the operator_pubkey field has been changed to bosd::Descriptor but the field name and the associated API docs have not been changed. It's fine if you plan to do this in later commits. If that is the case, you can set this PR to draft and still request for early feedback.
3. The ticket is specifically about changing the payout transactions . So I'd have expected to see changes only in those transactions (namely, payout and payout_optimistic). I think the reason it affects so many files is because this PR makes the CPFP connector's address generation function fallible. This is not bad per se but I would like you to think about why converting a descriptor to an address would fail and how would the caller handle that error, and so if it is reasonable to even propagate that error from the connector to the transactions to whichever crate is using those transactions.

[sistemd]

1. I am looking at the definition of atomic commits in our Notion.

• Single Purpose: Each commit addresses exactly one concern
• Self-Contained: The codebase should compile and tests should pass after each commit
• Complete: All changes necessary for the feature/fix are included
• Minimal: No unrelated changes are included

In particular, all changes in my commit are required for the code to compile. Setting an upper limit on no. of files changed (I don't see any such limit in our definition of atomic commits) is in this case a contradiction to the second point above. Let me know how you exactly want me to split the commit, and still make it compile, and I will gladly do it!

2. Which documentation needs to change? AFAIU, the doc comment is still correct. The type has changed, but this is just a refactor, there is no real change in functionality.

I think the reason it affects so many files is because this PR makes the CPFP connector's address generation function fallible. ... Why converting a descriptor to an address would fail?

A bosd::Descriptor can be one of many different things internally. But in the context of operator_pubkey, it is only expected to be a XOnlyPublicKey internally. Therefore, if the configuration is incorrect, and the user passed a different type of descriptor, operations on the descriptor can fail. Of course, we can do this validation early, and then unwrap later. The end result is that we replaced a more precise type (XOnlyPublicKey) with a less precise type (bosd::Descriptor) and switched from compile-time guarantees to runtime checking. Would you prefer me to do it that way?

[Rajil1213]

In particular, all changes in my commit are required for the code to compile. Setting an upper limit on no. of files changed (I don't see any such limit in our definition of atomic commits) is in this case a contradiction to the second point above. Let me know how you exactly want me to split the commit, and still make it compile, and I will gladly do it!

You're right in that atomic commits are not defined by the number of files changed but there is more than one way to make the code compile while making sure that commits are small enough to make reviews easier. For example, you could start by changing the XOnlyPublicKey to bosd::Descriptor and yet, not change the .public_key() API on the CpfpConnector (you could .expect() it and eat up any errors). You can then reason about whether it makes sense to propagate the error to the caller and decide to include any such changes in the next commit. Then, you could have a separate commit for any documentation changes (then, any review comments on the documentation can be amended onto this comment). I do not want to be too prescriptive here but this is how I'd go about splitting commits.

Which documentation needs to change? AFAIU, the doc comment is still correct. The type has changed, but this is just a refactor, there is no real change in functionality.

There is a change in functionality. This is a strictly more useful API than the previous one. In fact, now that I think about it, this PR might need to be redone. As far as I can see, the API accepts a descriptor but ultimately converts it to a P2TR address. The purpose of using descriptors is to allow for any address type (not just P2TR/taproot addresses). For the payout transactions, this can technically also be an OP_RETURN.

...if the configuration is incorrect, and the user passed a different type of descriptor, operations on the descriptor can fail. Of course, we can do this validation early, and then unwrap later. The end result is that we replaced a more precise type (XOnlyPublicKey) with a less precise type (bosd::Descriptor) and switched from compile-time guarantees to runtime checking. Would you prefer me to do it that way?

Yes. This is a fairly low level implementation detail and the current API expects a developer to trace their way all the way from the top of the stack to the very bottom where the descriptor to public key conversion would fail. You might not even need this conversion because as far as I can tell, the only reason we are getting this error is because we are going from Descriptor to public key to taproot address whereas we should just go directly to the address instead of enforcing the use of taproot address's (as mentioned in the earlier comment).

I've done a second pass of this PR to point out some of the things I mentioned in the earlier review. I haven't gone through all the changes though so use your discretion on the rest of the changes as well.

The biggest blocker for this PR though is the change required to go from P2TR addresses to just descriptors.

[sistemd]

Thanks for your review @Rajil1213!

I largely re-did the PR, leaving a single commit right now, without error handling. Error handling will be added in a follow-up commit like you suggested.

The number of changed files is pretty big, but most of it is renames, .clone() calls, and superficial changes necessary in order to satisfy the compiler. The most important changes are where descriptor_to_x_only_pubkey is being called. This call remains necessary even after the changes because in some code paths, the operator descriptor (fka operator pubkey) has to be used as an internal key of a taproot, which AFAIU has to be an XOnlyPublicKey, there's no way around it. Is this correct? Do the descriptor_to_x_only_pubkey calls seem correct to you, or am I missing something?

[storopoli]

ACK 16f79ec

Agree with @Rajil1213's comments on atomicity and have commits being very specific.

[Rajil1213]

We're making a lot of progress here but I've left a few comments on the ergonomics of some of the changes and on some matters of hygiene.

[sistemd]

Thanks for your review @Rajil1213! I have addressed your comments. To avoid redundant work, I have still not implemented error handling. Let me know if the XOnlyPublicKey/Descriptor boundary looks correct to you now. Once it is correct I will implement error handling.

[Rajil1213]

Some final set of changes and I think, we should be good to go.

[Rajil1213]

Is this regression still valid?

[sistemd]

I think we can probably ignore the regressions in git, right? b19eab6

[Rajil1213]

Nice!

Now, try commenting out the generate_address() method and see if there is anywhere in the codebase where this is used directly. If it isn't used anywhere, then we can remove descriptor_address from the ConnectorCpfp struct.

[sistemd]

We can't really remove descriptor_address because generate_locking_script uses it. But we can actually remove the generate_address fn: 2978fa9

[Rajil1213]

This should stay as descriptor. This struct holds all the information required to generate the transaction graph. Ultimately, this feeds into the graph API (aka the generate_graph function in the tx-graph crate). That API expects a descriptor (I think).

[sistemd]

c6c949b

[sistemd]

Thanks for the review! Here's the error handling commit: 105a45f.