wallet, rpc: add anti-fee-sniping to `send` and `sendall`

ishaanam commented at 10:43 pm on November 26, 2023: contributor

Currently, send and sendall don’t do anti-fee-sniping because they don’t use CreateTransaction. This PR adds anti-fee-sniping to these RPCs by calling DiscourageFeeSniping from FinishTransaction when the user does not specify a locktime.

DrahtBot commented at 10:43 pm on November 26, 2023: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	S3RK
Stale ACK	achow101, murchandamus

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#28710 (Remove the legacy wallet and BDB dependency by achow101)
#24128 (wallet: BIP 326 sequence based anti-fee-snipe for taproot inputs by maflcko)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

DrahtBot added the label CI failed on Nov 26, 2023

ishaanam force-pushed on Nov 27, 2023

DrahtBot removed the label CI failed on Nov 27, 2023

S3RK commented at 8:43 am on November 27, 2023: contributor

Concept ACK

achow101 commented at 3:24 pm on November 28, 2023: member

ACK 5c9ac1456b7b4512845270cd850e5c33b643f575

A test for send would be nice as well.

DrahtBot requested review from S3RK on Nov 28, 2023

ishaanam force-pushed on Nov 29, 2023

ishaanam commented at 5:05 pm on November 29, 2023: contributor

A test for send would be nice as well.

Done

in test/functional/wallet_send.py:145 in 5101f054ce outdated

141@@ -142,7 +142,12 @@ def test_send(self, from_wallet, to_wallet=None, amount=None, data=None,
142             return
143 
144         if locktime:
145+            assert_equal(from_wallet.decoderawtransaction(from_wallet.gettransaction(res["txid"])["hex"])["locktime"], locktime)

achow101 commented at 9:42 pm on December 4, 2023:

In 5101f054ce435ac653a3f5e780fca066e2f25088 “test: test sendall and send do anti-fee-sniping”

nit: gettransaction has a verbose argument that does the decoding so this doesn’t need to call decoderawtransaction. Same below.

ishaanam commented at 11:42 pm on December 4, 2023:

Done

in test/functional/wallet_sendall.py:392 in 5101f054ce outdated

387+
388+        assert_greater_than(tx_from_wallet["decoded"]["locktime"], tx_from_wallet["blockheight"] - 100)
389+
390+        self.add_utxos([10,11])
391+        txid = self.wallet.sendall(recipients=[self.remainder_target], options={"locktime":0})["txid"]
392+        tx_from_wallet = self.wallet.decoderawtransaction(self.wallet.getrawtransaction(txid))

achow101 commented at 9:43 pm on December 4, 2023:

In 5101f054ce435ac653a3f5e780fca066e2f25088 “test: test sendall and send do anti-fee-sniping”

nit: getrawtransaction has a verbosity option that will do the decoding, so no need to call decoderawtransaction here.

ishaanam commented at 11:42 pm on December 4, 2023:

Done

in test/functional/wallet_send.py:150 in 5101f054ce outdated

141@@ -142,7 +142,12 @@ def test_send(self, from_wallet, to_wallet=None, amount=None, data=None,
142             return
143 
144         if locktime:
145+            assert_equal(from_wallet.decoderawtransaction(from_wallet.gettransaction(res["txid"])["hex"])["locktime"], locktime)
146             return res
147+        else:
148+            if add_to_wallet:
149+                decoded_tx = from_wallet.decoderawtransaction(from_wallet.gettransaction(res["txid"])["hex"])
150+                assert_greater_than(decoded_tx["locktime"], 100)

achow101 commented at 9:43 pm on December 4, 2023:

In 5101f054ce435ac653a3f5e780fca066e2f25088 “test: test sendall and send do anti-fee-sniping”

Shouldn’t this check for getblockcount() - 100?

ishaanam commented at 11:42 pm on December 4, 2023:

Done

ishaanam force-pushed on Dec 4, 2023

in src/wallet/spend.h:220 in f348eadadd outdated

212@@ -213,6 +213,7 @@ struct CreatedTransactionResult
213         : tx(_tx), fee(_fee), fee_calc(_fee_calc), change_pos(_change_pos) {}
214 };
215 
216+void DiscourageFeeSniping(CMutableTransaction& tx, FastRandomContext& rng_fast, interfaces::Chain& chain, const uint256& block_hash, int block_height);

luke-jr commented at 6:00 pm on December 6, 2023:

nit: blank line under this; ideally docs above

ishaanam commented at 4:09 am on December 10, 2023:

Done

in test/functional/wallet_send.py:149 in f348eadadd outdated

141@@ -142,7 +142,12 @@ def test_send(self, from_wallet, to_wallet=None, amount=None, data=None,
142             return
143 
144         if locktime:
145+            assert_equal(from_wallet.gettransaction(txid=res["txid"], verbose=True)["decoded"]["locktime"], locktime)
146             return res
147+        else:
148+            if add_to_wallet:
149+                decoded_tx = from_wallet.decoderawtransaction(from_wallet.gettransaction(res["txid"])["hex"])

achow101 commented at 9:56 pm on December 7, 2023:

In f348eadadd6099e7763f04eb3d42cfd8bba33146 “test: test sendall and send do anti-fee-sniping”

nit: verbose here too.

ishaanam commented at 4:09 am on December 10, 2023:

Done

achow101 commented at 9:56 pm on December 7, 2023: member

ACK f348eadadd6099e7763f04eb3d42cfd8bba33146

ishaanam force-pushed on Dec 9, 2023

DrahtBot added the label CI failed on Dec 11, 2023

ishaanam force-pushed on Dec 22, 2023

murchandamus commented at 7:29 pm on December 27, 2023: contributor

Concept ACK and almost crACK d76f88051524971def5137a07ce2940dce1c33a9 except that there seems to be a type mix-up:

0 wallet/rpc/spend.cpp:1299:20: error: no matching function for call to 'FinishTransaction'
1            return FinishTransaction(pwallet, options, CMutableTransaction(*txr.tx));
2                   ^~~~~~~~~~~~~~~~~
3wallet/rpc/spend.cpp:79:17: note: candidate function not viable: expects an lvalue for 3rd argument
4static UniValue FinishTransaction(const std::shared_ptr<CWallet> pwallet, const UniValue& options, CMutableTransaction& rawTx)
5                ^
61 error generated.

ishaanam force-pushed on Dec 27, 2023

ishaanam commented at 8:19 pm on December 27, 2023: contributor

Fixed silent merge conflict.

DrahtBot removed the label CI failed on Dec 27, 2023

murchandamus commented at 9:06 pm on December 27, 2023: contributor

ACK c8de459de6d22e37164583f078747facaeae69c6

DrahtBot requested review from achow101 on Dec 27, 2023

DrahtBot added the label CI failed on Jan 16, 2024

DrahtBot added the label Needs rebase on Jan 23, 2024

ishaanam force-pushed on Jan 25, 2024

DrahtBot removed the label Needs rebase on Jan 25, 2024

ishaanam force-pushed on Jan 29, 2024

luke-jr referenced this in commit 7c481a931c on Jan 30, 2024

luke-jr referenced this in commit d036e53caa on Jan 30, 2024

murchandamus commented at 8:04 pm on February 1, 2024: contributor

I circled back to review, I think this has a merge conflict

luke-jr changes_requested

luke-jr commented at 5:38 pm on March 4, 2024: member

0--- a/test/functional/wallet_import_rescan.py
1+++ b/test/functional/wallet_import_rescan.py
2@@ -293,6 +293,7 @@ class ImportRescanTest(BitcoinTestFramework):
3                 add_to_wallet=False,
4                 inputs=[unspent_txid_map[variant.initial_txid]],
5                 outputs=[{ADDRESS_BCRT1_UNSPENDABLE : variant.initial_amount}],
6+                locktime=0,
7                 subtract_fee_from_outputs=[0]
8             )
9             variant.child_txid = child["txid"]

luke-jr referenced this in commit 4cf3a4a73b on Mar 5, 2024

luke-jr referenced this in commit 2200dfd225 on Mar 5, 2024

ishaanam force-pushed on Mar 6, 2024

DrahtBot removed the label CI failed on Mar 6, 2024

luke-jr referenced this in commit 13e36c822b on Mar 13, 2024

luke-jr referenced this in commit a20609e70a on Mar 13, 2024

in src/wallet/rpc/spend.cpp:97 in 8e5778a9fb outdated

93 {
94+    bool can_anti_fee_snipe = !options.exists("locktime");
95+
96+    for (const CTxIn& tx_in : rawTx.vin) {
97+        // Can not be FINAL for locktime to work
98+        can_anti_fee_snipe &= tx_in.nSequence != CTxIn::SEQUENCE_FINAL;

murchandamus commented at 8:08 pm on March 13, 2024:

In 8e5778a9fb6fbcbddd4ba87cc3f3caf41118592a (wallet, rpc: add anti-fee-sniping to send and sendall):

It seems to me that the nSequence check here is too restrictive. Locktime is disabled when all sequences are set to final, but it seems to me that can_anti_fee_snipe would be set to false here, even if only one input’s sequence is set to final.

I’m not sure this would matter much in practice because most transactions would probably use the same sequence value for all inputs, but can_anti_fee_snipe should probably end up true if at least one nSequence is non-final?

Could you perhaps add a test that a transaction with final nSequence values does not get its locktime set, but one with mixed values and one with all non-final nSequence values both do?

achow101 commented at 9:48 pm on April 2, 2024:

It seems to me that the nSequence check here is too restrictive. Locktime is disabled when all sequences are set to final, but it seems to me that can_anti_fee_snipe would be set to false here, even if only one input’s sequence is set to final.

Currently DiscourageFeeSniping will assert if any sequence is final, so this check is appropriate for now. It also enforces that the sequences are either MAX_SEQUENCE_NONFINAL or MAX_BIP125_RBF_SEQUENCE, so this needs to be checking that as well otherwise an assertion will be hit. Or a commit could be added to relax those checks.

Could you perhaps add a test that a transaction with final nSequence values does not get its locktime set, but one with mixed values and one with all non-final nSequence values both do?

I would also like to see tests for other sequence numbers as there is a risk of reaching an assertion here.

S3RK commented at 7:05 am on June 19, 2024:

It also enforces that the sequences are either MAX_SEQUENCE_NONFINAL or MAX_BIP125_RBF_SEQUENCE, so this needs to be checking that as well otherwise an assertion will be hit. Or a commit could be added to relax those checks.

It seems like this part of the comment wasn’t addressed

ishaanam commented at 7:08 pm on June 27, 2024:

It also enforces that the sequences are either MAX_SEQUENCE_NONFINAL or MAX_BIP125_RBF_SEQUENCE, so this needs to be checking that as well otherwise an assertion will be hit. Or a commit could be added to relax those checks

I have added these checks to FinishTransaction.

Could you perhaps add a test that a transaction with final nSequence values does not get its locktime set, but one with mixed values and one with all non-final nSequence values both do?

I have added a test for this.

ishaanam commented at 9:44 pm on March 14, 2024: contributor

0--- a/test/functional/wallet_import_rescan.py
1+++ b/test/functional/wallet_import_rescan.py
2@@ -293,6 +293,7 @@ class ImportRescanTest(BitcoinTestFramework):
3                 add_to_wallet=False,
4                 inputs=[unspent_txid_map[variant.initial_txid]],
5                 outputs=[{ADDRESS_BCRT1_UNSPENDABLE : variant.initial_amount}],
6+                locktime=0,
7                 subtract_fee_from_outputs=[0]
8             )
9             variant.child_txid = child["txid"]

Done

achow101 removed review request from achow101 on Apr 16, 2024

DrahtBot added the label Needs rebase on Jun 5, 2024

murchandamus commented at 8:45 pm on June 11, 2024: contributor

0--- a/test/functional/wallet_import_rescan.py
1+++ b/test/functional/wallet_import_rescan.py
2@@ -293,6 +293,7 @@ class ImportRescanTest(BitcoinTestFramework):
3                 add_to_wallet=False,
4                 inputs=[unspent_txid_map[variant.initial_txid]],
5                 outputs=[{ADDRESS_BCRT1_UNSPENDABLE : variant.initial_amount}],
6+                locktime=0,
7                 subtract_fee_from_outputs=[0]
8             )
9             variant.child_txid = child["txid"]

Done

It seems that you may have been working on an update, but it might not have been pushed. Is that possible?

ishaanam commented at 9:24 pm on June 17, 2024: contributor

0--- a/test/functional/wallet_import_rescan.py
1+++ b/test/functional/wallet_import_rescan.py
2@@ -293,6 +293,7 @@ class ImportRescanTest(BitcoinTestFramework):
3                 add_to_wallet=False,
4                 inputs=[unspent_txid_map[variant.initial_txid]],
5                 outputs=[{ADDRESS_BCRT1_UNSPENDABLE : variant.initial_amount}],
6+                locktime=0,
7                 subtract_fee_from_outputs=[0]
8             )
9             variant.child_txid = child["txid"]

Done

It seems that you may have been working on an update, but it might not have been pushed. Is that possible?

I pushed this change, but I had forgotten to comment until later.

wallet, rpc: add anti-fee-sniping to `send` and `sendall` 6825ae0c78

test: test sendall and send do anti-fee-sniping b11d00d54e

in src/wallet/rpc/spend.cpp:108 in 8e5778a9fb outdated

 97+        // Can not be FINAL for locktime to work
 98+        can_anti_fee_snipe &= tx_in.nSequence != CTxIn::SEQUENCE_FINAL;
 99+    }
100+
101+    if (can_anti_fee_snipe) {
102+        LOCK(pwallet->cs_wallet);

S3RK commented at 7:10 am on June 19, 2024:

I’m not clear why do we need a wallet lock here?

ishaanam commented at 7:10 pm on June 27, 2024:

Here the wallet is locked because it runs CWallet::GetLastBlockHash and CWallet::GetLastBlockHeight which both require pwallet->cs_wallet to be locked.

ishaanam force-pushed on Jun 27, 2024

DrahtBot removed the label Needs rebase on Jun 27, 2024

wallet, rpc: add anti-fee-sniping to send and sendall #28944

Code Coverage

Reviews

Conflicts

wallet, rpc: add anti-fee-sniping to `send` and `sendall` #28944