Feat/add uhpo crate rework by jayendramadaram · Pull Request #10 · pool2win/braidpool

jayendramadaram · 2024-06-18T06:02:40Z

This Pr is First pr in series of broken Pr's from #9

Changes Made

Added payout-update Transaction builder.
Offers way to Schnorr Sign payout-update Transactions.
removed dependency from nigiri from testcase perspective.
includes unit tests for payout-update.

pool2win

@jayendramadaram - I left some initial comments that need to be addressed. I am still going through payout_update.rs and will follow up with another review.

pool2win · 2024-07-01T07:32:09Z

.gitignore

 venv
-target
+target
+todo


Move this to .git/config/ignore if this is your personal ignore file. Other contributors don't have this file.

pool2win · 2024-07-01T07:33:35Z

node/connection/src/lib.rs

    #[tokio::test]
-    // TODO: This is a smoke test and we need to improve how we wait for listen to complete before we invoke connect.
    async fn it_should_run_connect_without_errors() {
-        use tokio::time::{sleep, Duration};


Why did you remove these? This change doesn't seem related to the UHPO work, so if we need this change to go in, let's make it a separate PR.

pool2win · 2024-07-01T07:34:44Z

node/connection/src/lib.rs


-        // TODO: Fix this smoke test! Kill the sleep in this smoke test.
-        sleep(Duration::from_millis(100)).await;
+        notify.notified().await;


I think this change of notifier is valuable, but can you please move this into a separate PR so we can test and review it as a unit.

pool2win · 2024-07-01T07:36:27Z

node/uhpo/docs/spec.md

@@ -0,0 +1,320 @@
+# Braidpool UHPO Spec


We should not have this design doc in the repo. I am not sure the design will always remain in sync with the code, and thus will cause confusion. Let's move this into a gist and link the gist from the PR description. This way the history of this discussion is maintained and we are not burdened to keep it in sync with the code as it evolves.

pool2win · 2024-07-01T11:08:22Z

node/uhpo/src/error.rs

+impl std::error::Error for UhpoError {}
+
+impl UhpoError {
+    pub fn new_other_error(message: &str) -> Self {


Why not simply call this method new?

pool2win · 2024-07-01T11:52:11Z

I see what happened with the changes for notifier. It seems like there is a problem with this PR where the commits from stutxo have crept into this PR. We need to clear this up, so that only your commits relevant to this PR are in this PR.

pool2win

My second review here, about using ownership transfer instead of references with lifetimes.

pool2win · 2024-07-01T11:55:44Z

node/uhpo/src/payout_update.rs

+
+impl<'a> PayoutUpdate<'a> {
+    pub fn new(
+        prev_update_tx: Option<&'a Transaction>,


I think we can let these arguments can be passed in without using references. I think the client can be required to transfer ownership to the payout update value. If the client wants to retain ownership, then they can Clone the value and pass it in here.

The reason to use ownership transfer instead of references is to keep the code simpler and therefore easier to maintain and use in the future.

Let's try to do this, and I can review again.

I've implemented the changes as suggested,

Removed lifetime parameters from the PayoutUpdate struct.

Modified the PayoutUpdate struct to take ownership of Transaction objects

Adjusted internal methods to work with owned TxOut instances

pub struct PayoutUpdate { transaction: Transaction, coinbase_txout: TxOut, prev_update_txout: Option<TxOut>, } impl PayoutUpdate { pub fn new( prev_update_tx: Option<Transaction>, coinbase_tx: Transaction, next_out_address: Address, projected_fee: Amount, ) -> Result<Self, Error> { let prev_update_txout = prev_update_tx.as_ref().map(|tx| tx.output[0].clone()); let coinbase_txout = coinbase_tx.output[0].clone(); // ... rest of the implementation ... } // ... other methods updated accordingly ... }

jayendramadaram · 2024-07-02T19:16:51Z

I have, made above suggested changes, latest commit includes

removal of personal ignore files from .gitignore
removal of spec docs
This PR is rebased to remove commits from stutxo
Modified the PayoutUpdate struct to take ownership of Transaction objects

pool2win

Nice progress. A few more things we need to do. See individual comments.

pool2win · 2024-07-03T10:50:04Z

.gitignore

 simulations/*.toolbox
-venv
-target
+venv


The venv should be in your private .git/info/exclude

pool2win · 2024-07-03T11:41:56Z

node/uhpo/src/payout_update.rs

+    coinbase_tx: &Transaction,
+    prev_update_tx: Option<&Transaction>,
+    next_out_address: Address,
+    projected_fee: Amount,
+    coinbase_txout: &TxOut,
+    prev_update_txout: Option<&TxOut>,


We don't need any of these arguments to be a reference.

Here's a working patch that shows how. 638d3c7

pool2win · 2024-07-03T11:42:43Z

node/uhpo/src/payout_update.rs

+    let mut payout_update_tx = Transaction {
+        version: Version::TWO,
+        lock_time: LockTime::ZERO,
+        input: vec![],
+        output: vec![],
+    };
+
+    payout_update_tx.input.push(TxIn {
+        previous_output: OutPoint {
+            txid: coinbase_tx.compute_txid(),
+            vout: 0,
+        },
+        script_sig: ScriptBuf::new(),
+        sequence: Sequence::MAX,
+        witness: Witness::default(),
+    });
+
+    if let Some(tx) = prev_update_tx {
+        payout_update_tx.input.push(TxIn {
+            previous_output: OutPoint {
+                txid: tx.compute_txid(),
+                vout: 0,
+            },
+            script_sig: ScriptBuf::new(),
+            sequence: Sequence::MAX,
+            witness: Witness::default(),
+        });
+    }
+
+    payout_update_tx.output.push(TxOut {
+        value: total_amount - projected_fee,
+        script_pubkey: next_out_address.script_pubkey(),
+    });
+
+    Ok(payout_update_tx)


There are a lot of individual steps we are taking here. Maybe we should use a builder pattern here again and each of the separate blocks can be turned into separate functions? That will also result in the build_transaction function either disappearing or getting simpler.

https://rust-unofficial.github.io/patterns/patterns/creational/builder.html

I 'have Implemented changes as suggested,

added new TransactionBuilder which can be used but both PayoutSettlement and PayoutUpdate

pool2win · 2024-07-03T11:44:27Z

node/uhpo/src/payout_update.rs

+    prevouts: &[&TxOut],
+    private_key: &SecretKey,
+    tweak: &Option<Scalar>,


Again, references. My understanding right now with Rust is - use move, it is efficient in Rust, use refs only if really needed.

I have changes private_key from not to take as reference, but other two prevouts and tweak are meant to be passed as reference to rust-bitcoin's SighashCache.taproot_key_spend_signature_hash and Keypair.add_xonly_tweak, hence using continuing with reference only for these two params.

pool2win · 2024-07-03T11:49:21Z

node/uhpo/src/payout_update.rs

+    let secp = Secp256k1::new();
+
+    let keypair: Keypair = match tweak {
+        Some(tweak) => private_key.keypair(&secp).add_xonly_tweak(&secp, tweak)?,


We should differentiate the error here and the one on line 158, so that the client knows where the signing failed.

Added new errors as suggested

UhpoError::KeypairCreationError

UhpoError::SignatureVerificationError

pool2win · 2024-07-03T11:56:26Z

node/uhpo/src/payout_update.rs

+    }
+
+    #[test]
+    fn test_add_signature() {


We should add tests for the code paths where:

No tweak is passed

There is an error on line 140

There is an error on line 158

Add test where signature verification fails - you might need to use mocks for this

With latest changes made,

new testcases are added to cover tweak keypair case.

mock KeypairCreationError and SignatureVerificationError using mockall crate and making SecretKey , Secp256k1 and Keypair Generic.

pool2win · 2024-07-03T11:57:05Z

node/uhpo/src/payout_update.rs

+    }
+}
+
+fn build_transaction(


Add doc comment

node/uhpo/src/payout_update.rs

jayendramadaram · 2024-07-09T11:37:41Z

I have implemented the suggested changes in the latest commit. The updates include:

Added doc strings and comments to code .
Implemented a separate TransactionBuilder for constructing Bitcoin transactions in PayoutUpdate, improving modularity and reusability.
Moved add_signatures function to src/crypto/signature.rs, as it's used by both PayoutUpdate and PayoutSettlement.
Introduced new, more descriptive error types.
Implemented mock tests to simulate rust-bitcoin instances returning errors.

codecov-commenter · 2024-07-14T06:30:17Z

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 95.67780% with 22 lines in your changes missing coverage. Please review.

Project coverage is 93.11%. Comparing base (a2992c4) to head (3ed8a9f).

Files	Patch %	Lines
node/uhpo/src/error.rs	0.00%	18 Missing ⚠️
node/uhpo/src/payout_update.rs	98.45%	4 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@                Coverage Diff                 @@
##           add-uhpo-crate      #10      +/-   ##
==================================================
+ Coverage           91.71%   93.11%   +1.40%     
==================================================
  Files                   9       13       +4     
  Lines                 929     1438     +509     
==================================================
+ Hits                  852     1339     +487     
- Misses                 77       99      +22

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

pool2win

Nice! I am still not sure if the new approach adds a function call overhead - I'll look at that again.

For now a few things:

CI is failing.
We should add the failure/success test cases appropriately. I left comments in the PR review for where we need more tests.

pool2win · 2024-07-14T06:23:47Z

node/uhpo/src/crypto/signature.rs

+    }
+
+    #[test]
+    fn test_add_signatures_key_creation_fails() {


Let's rename test to

test_add_signature_with_invalid_tweak_error_signature_creation_should_fail

basically

test_<fn>_<under_which_conditions>_<what_should_happen>

pool2win · 2024-07-14T06:26:09Z

node/uhpo/src/crypto/signature.rs

+    }
+
+    #[test]
+    fn test_add_signatures_signature_verification_fails() {


fn name here again,

test_add_signature_with_a_bad_signature_should_fail_during_verification

node/uhpo/src/crypto/signature.rs

node/uhpo/src/payout_update.rs

jayendramadaram · 2024-07-15T14:08:18Z

Fixes made from latest commit,

changed testcase names as requested
added a mock error testcase which mocks KeyCreationError(_) on add_coinbase_sig and add_prev_update_sig methods on PayoutUpdate. I feel single mock testcase should be fine, ignoring testcase for SignatureVerificationError.

feat/added payout update

34da195

pool2win mentioned this pull request Jul 1, 2024

Feat/add uhpo crate #9

Closed

pool2win requested changes Jul 1, 2024

View reviewed changes

feat/added payout update transaction Builder

53496f0

jayendramadaram force-pushed the feat/add-uhpo-crate-v2 branch from 6f6701b to 53496f0 Compare July 2, 2024 18:33

fix/Refactor PayoutUpdate to use ownership transfer and improve crate

86f25c1

pool2win requested changes Jul 3, 2024

View reviewed changes

jayendramadaram added 4 commits July 8, 2024 14:24

feat/add new tx builder

8f3fc21

test/add mock test for key creation

9a26b2a

test/add sig verification mock tests and Doc comments

581a2d1

test/add sig verification mock tests and Doc comments

415e32e

pool2win requested changes Jul 14, 2024

View reviewed changes

test/refractor testcase func handles

3ed8a9f

jayendramadaram requested a review from pool2win July 15, 2024 14:16

pool2win merged commit a4226d6 into pool2win:add-uhpo-crate Jul 15, 2024

jayendramadaram mentioned this pull request Jul 17, 2024

Feat/add Payout Settlement Transaction. #12

Open

               venv
-              target

                
                    No newline at end of file
+              target
+              todo

                
                    No newline at end of file

Conversation

jayendramadaram commented Jun 18, 2024

Changes Made

Uh oh!

pool2win left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pool2win commented Jul 1, 2024

Uh oh!

pool2win left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jayendramadaram commented Jul 2, 2024

Uh oh!

pool2win left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jayendramadaram commented Jul 9, 2024

Uh oh!

codecov-commenter commented Jul 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

pool2win left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jayendramadaram commented Jul 15, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

codecov-commenter commented Jul 14, 2024 •

edited

Loading