From f14c3a7f75f4483e83da5a6dfd2dc0cb3624133a Mon Sep 17 00:00:00 2001 From: jp1ac4 <121959000+jp1ac4@users.noreply.github.com> Date: Wed, 27 Mar 2024 17:59:16 +0000 Subject: [PATCH 1/5] spend: fix typo in enum variant --- src/spend.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/spend.rs b/src/spend.rs index 0118ef0b..dc7e543c 100644 --- a/src/spend.rs +++ b/src/spend.rs @@ -543,7 +543,7 @@ pub enum SpendTxFees { #[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] pub enum CreateSpendWarning { ChangeAddedToFee(u64), - AddtionalFeeForAncestors(u64), + AdditionalFeeForAncestors(u64), } impl fmt::Display for CreateSpendWarning { @@ -555,7 +555,7 @@ impl fmt::Display for CreateSpendWarning { amt, if *amt > 1 {"s"} else {""}, ), - CreateSpendWarning::AddtionalFeeForAncestors(amt) => write!( + CreateSpendWarning::AdditionalFeeForAncestors(amt) => write!( f, "An additional fee of {} sat{} has been added to pay for ancestors at the target feerate.", amt, @@ -737,7 +737,7 @@ pub fn create_spend( } if fee_for_ancestors.to_sat() > 0 { - warnings.push(CreateSpendWarning::AddtionalFeeForAncestors( + warnings.push(CreateSpendWarning::AdditionalFeeForAncestors( fee_for_ancestors.to_sat(), )); } From deb75884bfe8a47c9143fd7006960c81d956d6d4 Mon Sep 17 00:00:00 2001 From: jp1ac4 <121959000+jp1ac4@users.noreply.github.com> Date: Thu, 28 Mar 2024 10:00:47 +0000 Subject: [PATCH 2/5] tests: run black --- tests/test_framework/signer.py | 4 +++- tests/test_framework/utils.py | 4 +++- tests/test_misc.py | 7 +++++-- tests/test_spend.py | 24 +++++++++++++++++++----- 4 files changed, 30 insertions(+), 9 deletions(-) diff --git a/tests/test_framework/signer.py b/tests/test_framework/signer.py index b0332eb9..51c8e0f2 100644 --- a/tests/test_framework/signer.py +++ b/tests/test_framework/signer.py @@ -93,7 +93,9 @@ def sign_psbt_taproot(psbt, hds): psbt_str = psbt.to_base64() for hd in hds: xprv = hd.get_xpriv() - proc = subprocess.run([bin_path, psbt_str, xprv], capture_output=True, check=True) + proc = subprocess.run( + [bin_path, psbt_str, xprv], capture_output=True, check=True + ) psbt_str = proc.stdout.decode("utf-8") return PSBT.from_base64(psbt_str) diff --git a/tests/test_framework/utils.py b/tests/test_framework/utils.py index ffa50aa3..17b2c0ce 100644 --- a/tests/test_framework/utils.py +++ b/tests/test_framework/utils.py @@ -24,7 +24,9 @@ DEFAULT_BITCOIND_PATH = "bitcoind" BITCOIND_PATH = os.getenv("BITCOIND_PATH", DEFAULT_BITCOIND_PATH) OLD_LIANAD_PATH = os.getenv("OLD_LIANAD_PATH", None) IS_NOT_BITCOIND_24 = bool(int(os.getenv("IS_NOT_BITCOIND_24", True))) -USE_TAPROOT = bool(int(os.getenv("USE_TAPROOT", False))) # TODO: switch to True in a couple releases. +USE_TAPROOT = bool( + int(os.getenv("USE_TAPROOT", False)) +) # TODO: switch to True in a couple releases. COIN = 10**8 diff --git a/tests/test_misc.py b/tests/test_misc.py index c7f29a6d..fecad7c3 100644 --- a/tests/test_misc.py +++ b/tests/test_misc.py @@ -250,12 +250,15 @@ def test_coinbase_deposit(lianad, bitcoind): assert coin["is_change"] bitcoind.generate_block(100) wait_for_sync() - coin = next(c for c in lianad.rpc.listcoins()["coins"] if c["outpoint"] == coin["outpoint"]) + coin = next( + c for c in lianad.rpc.listcoins()["coins"] if c["outpoint"] == coin["outpoint"] + ) assert not coin["is_immature"] and coin["block_height"] is not None @pytest.mark.skipif( - OLD_LIANAD_PATH is None or USE_TAPROOT, reason="Need the old lianad binary to create the datadir." + OLD_LIANAD_PATH is None or USE_TAPROOT, + reason="Need the old lianad binary to create the datadir.", ) def test_migration(lianad_multisig, bitcoind): """Test we can start a newer lianad on a datadir created by an older lianad.""" diff --git a/tests/test_spend.py b/tests/test_spend.py index 9c496386..deeefc32 100644 --- a/tests/test_spend.py +++ b/tests/test_spend.py @@ -1,6 +1,12 @@ from fixtures import * from test_framework.serializations import PSBT, uint256_from_str -from test_framework.utils import sign_and_broadcast_psbt, wait_for, COIN, RpcError, USE_TAPROOT +from test_framework.utils import ( + sign_and_broadcast_psbt, + wait_for, + COIN, + RpcError, + USE_TAPROOT, +) def test_spend_change(lianad, bitcoind): @@ -334,7 +340,9 @@ def test_coin_selection(lianad, bitcoind): bytes.fromhex(spend_txid_1)[::-1] ) anc_vsize = bitcoind.rpc.getmempoolentry(spend_txid_1)["ancestorsize"] - anc_fees = int(bitcoind.rpc.getmempoolentry(spend_txid_1)["fees"]["ancestor"] * COIN) + anc_fees = int( + bitcoind.rpc.getmempoolentry(spend_txid_1)["fees"]["ancestor"] * COIN + ) additional_fee = additional_fees(anc_vsize, anc_fees, feerate) assert len(spend_res_2["warnings"]) == 1 assert ( @@ -352,7 +360,9 @@ def test_coin_selection(lianad, bitcoind): bytes.fromhex(spend_txid_1)[::-1] ) anc_vsize = bitcoind.rpc.getmempoolentry(spend_txid_1)["ancestorsize"] - anc_fees = int(bitcoind.rpc.getmempoolentry(spend_txid_1)["fees"]["ancestor"] * COIN) + anc_fees = int( + bitcoind.rpc.getmempoolentry(spend_txid_1)["fees"]["ancestor"] * COIN + ) additional_fee = additional_fees(anc_vsize, anc_fees, feerate) assert len(spend_res_2["warnings"]) == 1 assert ( @@ -398,8 +408,12 @@ def test_coin_selection(lianad, bitcoind): anc_vsize = bitcoind.rpc.getmempoolentry(deposit_2)["ancestorsize"] anc_fees = int(bitcoind.rpc.getmempoolentry(deposit_2)["fees"]["ancestor"] * COIN) prev_anc_vsize = bitcoind.rpc.getmempoolentry(spend_txid_1)["ancestorsize"] - prev_anc_fees = int(bitcoind.rpc.getmempoolentry(spend_txid_1)["fees"]["ancestor"] * COIN) - additional_fee = additional_fees(anc_vsize, anc_fees, feerate) + additional_fees(prev_anc_vsize, prev_anc_fees, feerate) + prev_anc_fees = int( + bitcoind.rpc.getmempoolentry(spend_txid_1)["fees"]["ancestor"] * COIN + ) + additional_fee = additional_fees(anc_vsize, anc_fees, feerate) + additional_fees( + prev_anc_vsize, prev_anc_fees, feerate + ) assert len(spend_res_3["warnings"]) == 1 assert ( spend_res_3["warnings"][0] From 8078a2791a81eecd7f662e4fd43965e185c1737e Mon Sep 17 00:00:00 2001 From: jp1ac4 <121959000+jp1ac4@users.noreply.github.com> Date: Thu, 28 Mar 2024 10:49:07 +0000 Subject: [PATCH 3/5] tests: use function to calculate additional fee This will reduce flakiness in case the ancestor size differs due to changes in signature size. --- tests/test_spend.py | 27 +++++++++++++-------------- 1 file changed, 13 insertions(+), 14 deletions(-) diff --git a/tests/test_spend.py b/tests/test_spend.py index deeefc32..76646183 100644 --- a/tests/test_spend.py +++ b/tests/test_spend.py @@ -9,6 +9,15 @@ from test_framework.utils import ( ) +def additional_fees(anc_vsize, anc_fee, target_feerate): + """The additional fee which must have been computed by lianad.""" + computed_anc_vsize = int(anc_fee / target_feerate) + print(f"c ", computed_anc_vsize) + extra_vsize = anc_vsize - computed_anc_vsize + print("e ", extra_vsize) + return extra_vsize * target_feerate + + def test_spend_change(lianad, bitcoind): """We can spend a coin that was received on a change address.""" # Receive a coin on a receive address @@ -179,15 +188,13 @@ def test_coin_marked_spent(lianad, bitcoind): psbt = PSBT.from_base64(res["psbt"]) sign_and_broadcast(psbt) assert len(psbt.o) == 4 - assert bitcoind.rpc.getmempoolentry(deposit_d)["ancestorsize"] == 165 - assert bitcoind.rpc.getmempoolentry(deposit_d)["fees"]["ancestor"] * COIN == 165 - # ancestor vsize at feerate 2 sat/vb = ancestor_fee / 2 = 165 / 2 = 82 - # extra_weight <= (extra vsize * witness factor) = (165 - 82) * 4 = 332 - # additional fee at 2 sat/vb (0.5 sat/wu) = 332 * 0.5 = 166 + anc_vsize = bitcoind.rpc.getmempoolentry(deposit_d)["ancestorsize"] + anc_fees = int(bitcoind.rpc.getmempoolentry(deposit_d)["fees"]["ancestor"] * COIN) + additional_fee = additional_fees(anc_vsize, anc_fees, 2) assert len(res["warnings"]) == 1 assert ( res["warnings"][0] - == "An additional fee of 166 sats has been added to pay for ancestors at the target feerate." + == f"An additional fee of {additional_fee} sats has been added to pay for ancestors at the target feerate." ) # All the spent coins must have been detected as such @@ -322,14 +329,6 @@ def test_coin_selection(lianad, bitcoind): dest_addr_2 = bitcoind.rpc.getnewaddress() # If feerate is higher than ancestor, we'll need to pay extra. - def additional_fees(anc_vsize, anc_fee, target_feerate): - """The additional fee which must have been computed by lianad.""" - computed_anc_vsize = int(anc_fee / target_feerate) - print(f"c ", computed_anc_vsize) - extra_vsize = anc_vsize - computed_anc_vsize - print("e ", extra_vsize) - return extra_vsize * target_feerate - # Try 10 sat/vb: feerate = 10 spend_res_2 = lianad.rpc.createspend({dest_addr_2: 10_000}, [], feerate) From 5d6ca97e4cf2e1778552d4980291eca1d2cc57c7 Mon Sep 17 00:00:00 2001 From: jp1ac4 <121959000+jp1ac4@users.noreply.github.com> Date: Thu, 28 Mar 2024 10:58:49 +0000 Subject: [PATCH 4/5] spend: reword warning messages --- src/commands/mod.rs | 27 ++++++++++++++++++++++++--- src/spend.rs | 13 +++++++++---- tests/test_spend.py | 24 ++++++++++++++++++------ 3 files changed, 51 insertions(+), 13 deletions(-) diff --git a/src/commands/mod.rs b/src/commands/mod.rs index f28cdbb4..94d7ea24 100644 --- a/src/commands/mod.rs +++ b/src/commands/mod.rs @@ -1571,7 +1571,14 @@ mod tests { ); assert_eq!(tx.output[0].value.to_sat(), 95_000); // change = 100_000 - 95_000 - /* fee without change */ 127 - /* extra fee for change output */ 43 = 4830 - assert_eq!(warnings, vec!["Change amount of 4830 sats added to fee as it was too small to create a transaction output."]); + assert_eq!( + warnings, + vec![ + "Dust UTXO. The minimal change output allowed by Liana is 5000 sats. \ + Instead of creating a change of 4830 sats, it was added to the \ + transaction fee. Select a larger input to avoid this from happening." + ] + ); // Increase the target value by the change amount and the warning will disappear. *destinations.get_mut(&dummy_addr).unwrap() = 95_000 + 4_830; @@ -1622,7 +1629,14 @@ mod tests { panic!("expect successful spend creation") }; // Message uses "sat" instead of "sats" when value is 1. - assert_eq!(warnings, vec!["Change amount of 1 sat added to fee as it was too small to create a transaction output."]); + assert_eq!( + warnings, + vec![ + "Dust UTXO. The minimal change output allowed by Liana is 5000 sats. \ + Instead of creating a change of 1 sat, it was added to the \ + transaction fee. Select a larger input to avoid this from happening." + ] + ); // Now decrease the target value so that we have enough for a change output. *destinations.get_mut(&dummy_addr).unwrap() = @@ -1651,7 +1665,14 @@ mod tests { } else { panic!("expect successful spend creation") }; - assert_eq!(warnings, vec!["Change amount of 4999 sats added to fee as it was too small to create a transaction output."]); + assert_eq!( + warnings, + vec![ + "Dust UTXO. The minimal change output allowed by Liana is 5000 sats. \ + Instead of creating a change of 4999 sats, it was added to the \ + transaction fee. Select a larger input to avoid this from happening." + ] + ); // Now if we mark the coin as spent, we won't create another Spend transaction containing // it. diff --git a/src/spend.rs b/src/spend.rs index dc7e543c..a90a0c67 100644 --- a/src/spend.rs +++ b/src/spend.rs @@ -551,15 +551,20 @@ impl fmt::Display for CreateSpendWarning { match self { CreateSpendWarning::ChangeAddedToFee(amt) => write!( f, - "Change amount of {} sat{} added to fee as it was too small to create a transaction output.", + "Dust UTXO. The minimal change output allowed by Liana is {} sats. \ + Instead of creating a change of {} sat{}, it was added to the \ + transaction fee. Select a larger input to avoid this from happening.", + DUST_OUTPUT_SATS, amt, - if *amt > 1 {"s"} else {""}, + if *amt > 1 { "s" } else { "" }, ), CreateSpendWarning::AdditionalFeeForAncestors(amt) => write!( f, - "An additional fee of {} sat{} has been added to pay for ancestors at the target feerate.", + "CPFP: an unconfirmed input was selected. The current transaction fee \ + was increased by {} sat{} to make the average feerate of both the input \ + and current transaction equal to the selected feerate.", amt, - if *amt > 1 {"s"} else {""}, + if *amt > 1 { "s" } else { "" }, ), } } diff --git a/tests/test_spend.py b/tests/test_spend.py index 76646183..d1fcb921 100644 --- a/tests/test_spend.py +++ b/tests/test_spend.py @@ -141,7 +141,9 @@ def test_coin_marked_spent(lianad, bitcoind): assert len(res["warnings"]) == 1 assert ( res["warnings"][0] - == f"Change amount of {change_amount} sats added to fee as it was too small to create a transaction output." + == "Dust UTXO. The minimal change output allowed by Liana is 5000 sats. " + f"Instead of creating a change of {change_amount} sats, it was added to the " + "transaction fee. Select a larger input to avoid this from happening." ) # Spend the third coin to an address of ours, no change @@ -158,7 +160,9 @@ def test_coin_marked_spent(lianad, bitcoind): assert len(res["warnings"]) == 1 assert ( res["warnings"][0] - == f"Change amount of {change_amount} sats added to fee as it was too small to create a transaction output." + == "Dust UTXO. The minimal change output allowed by Liana is 5000 sats. " + f"Instead of creating a change of {change_amount} sats, it was added to the " + "transaction fee. Select a larger input to avoid this from happening." ) # Spend the fourth coin to an address of ours, with change @@ -194,7 +198,9 @@ def test_coin_marked_spent(lianad, bitcoind): assert len(res["warnings"]) == 1 assert ( res["warnings"][0] - == f"An additional fee of {additional_fee} sats has been added to pay for ancestors at the target feerate." + == "CPFP: an unconfirmed input was selected. The current transaction fee " + f"was increased by {additional_fee} sats to make the average feerate of " + "both the input and current transaction equal to the selected feerate." ) # All the spent coins must have been detected as such @@ -346,7 +352,9 @@ def test_coin_selection(lianad, bitcoind): assert len(spend_res_2["warnings"]) == 1 assert ( spend_res_2["warnings"][0] - == f"An additional fee of {additional_fee} sats has been added to pay for ancestors at the target feerate." + == "CPFP: an unconfirmed input was selected. The current transaction fee " + f"was increased by {additional_fee} sats to make the average feerate of " + "both the input and current transaction equal to the selected feerate." ) # Try 3 sat/vb: @@ -366,7 +374,9 @@ def test_coin_selection(lianad, bitcoind): assert len(spend_res_2["warnings"]) == 1 assert ( spend_res_2["warnings"][0] - == f"An additional fee of {additional_fee} sats has been added to pay for ancestors at the target feerate." + == "CPFP: an unconfirmed input was selected. The current transaction fee " + f"was increased by {additional_fee} sats to make the average feerate of " + "both the input and current transaction equal to the selected feerate." ) # 2 sat/vb is same feerate as ancestor and we have no warnings: @@ -416,7 +426,9 @@ def test_coin_selection(lianad, bitcoind): assert len(spend_res_3["warnings"]) == 1 assert ( spend_res_3["warnings"][0] - == f"An additional fee of {additional_fee} sats has been added to pay for ancestors at the target feerate." + == "CPFP: an unconfirmed input was selected. The current transaction fee " + f"was increased by {additional_fee} sats to make the average feerate of " + "both the input and current transaction equal to the selected feerate." ) spend_psbt_3 = PSBT.from_base64(spend_res_3["psbt"]) spend_txid_3 = sign_and_broadcast_psbt(lianad, spend_psbt_3) From ed86696daf556533592b3285c70a2386a84ece71 Mon Sep 17 00:00:00 2001 From: jp1ac4 <121959000+jp1ac4@users.noreply.github.com> Date: Thu, 28 Mar 2024 12:38:09 +0000 Subject: [PATCH 5/5] tests: remove print statements --- tests/test_spend.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/tests/test_spend.py b/tests/test_spend.py index d1fcb921..dcfdc296 100644 --- a/tests/test_spend.py +++ b/tests/test_spend.py @@ -12,9 +12,7 @@ from test_framework.utils import ( def additional_fees(anc_vsize, anc_fee, target_feerate): """The additional fee which must have been computed by lianad.""" computed_anc_vsize = int(anc_fee / target_feerate) - print(f"c ", computed_anc_vsize) extra_vsize = anc_vsize - computed_anc_vsize - print("e ", extra_vsize) return extra_vsize * target_feerate