test: Add functional test for replacement relay fee check

ariard commented at 5:21 pm on June 22, 2021: member

This PR adds rename the reject_reason of our implementation of BIP125 rule 4 and adds missing functional test coverage. Note, insufficient fee is already the reject_reason of few others PreChecks replacement checks and as such might be confusing.

The replacement transaction must also pay for its own bandwidth at or above the rate set by the node’s minimum relay fee setting. For example, if the minimum relay fee is 1 satoshi/byte and the replacement transaction is 500 bytes total, then the replacement must pay a fee at least 500 satoshis higher than the sum of the originals.

 0        // Finally in addition to paying more fees than the conflicts the
 1        // new transaction must pay for its own bandwidth.
 2        CAmount nDeltaFees = nModifiedFees - nConflictingFees;
 3        if (nDeltaFees < ::incrementalRelayFee.GetFee(nSize))
 4        {
 5            return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "insufficient fee",
 6                    strprintf("rejecting replacement %s, not enough additional fees to relay; %s < %s",
 7                        hash.ToString(),
 8                        FormatMoney(nDeltaFees),
 9                        FormatMoney(::incrementalRelayFee.GetFee(nSize))));
10        }

in src/validation.cpp:907 in d19c7cccaa outdated

903@@ -904,7 +904,7 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
904         CAmount nDeltaFees = nModifiedFees - nConflictingFees;
905         if (nDeltaFees < ::incrementalRelayFee.GetFee(nSize))
906         {
907-            return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "insufficient fee",
908+            return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "insufficient replacement penalty",

MarcoFalke commented at 5:31 pm on June 22, 2021:

any reason to change the reject string? This will just cause a headache for downstream project (if they use the reject string) and might even be controversial due to the change “fee” -> “penalty”.

MarcoFalke commented at 5:33 pm on June 22, 2021:

Also the commit title is wrong? Mentions rule 5, but this is rule 4?

ariard commented at 5:47 pm on June 22, 2021:

Pushed new commit, updating to rule 4.

W.r.t to headache for downstream project, it’s motivated by the fact that this same reject reason is already used L897 for the “higher fee” check and L833 for the “higher feerate” check.

Do we want to dissipate the confusion ? I would say it’s better but can just drop it if you think it’s too bothering for downstream.

MarcoFalke commented at 5:50 pm on June 22, 2021:

If you really want to change it, a separate pull request with clear rationale to motivate the change (and the hassle it causes) would be better. Bundling it with a change that is adding a test might not be the best approach.

MarcoFalke commented at 5:55 pm on June 22, 2021:

Obviously it needs release notes etc. Though, I’d rather avoid breaking downstream via reject reasons, which might break silently because they are not an enum. #19339 (comment)

ariard commented at 6:10 pm on June 22, 2021:

Ah okay, well I’ll let the confusion for now and just happy to have test coverage. Updated the branch.

ariard force-pushed on Jun 22, 2021

DrahtBot added the label Needs rebase on Jun 22, 2021

ariard force-pushed on Jun 22, 2021

in test/functional/feature_rbf.py:643 in 801496453b outdated

638+
639+        # Higher fee, higher fee per KB, but the replacement does not provide a relay
640+        # fee conforming to node's `incrementalrelayfee` policy of 1000 sat per KB.
641+        replacement_transaction = CTransaction()
642+        replacement_transaction.vin = [CTxIn(outpoint, nSequence=0)]
643+        replacement_transaction.vout = [CTxOut(int(1 * COIN - 1), DUMMY_P2WPKH_SCRIPT)]

MarcoFalke commented at 6:23 pm on June 22, 2021:

how is this test different from the one above? # Should fail because we haven't changed the fee

MarcoFalke commented at 6:10 am on June 23, 2021:

It is different because the newly added test adds coverage for previously uncovered code https://marcofalke.github.io/btc_cov/total.coverage/src/validation.cpp.gcov.html

DrahtBot removed the label Needs rebase on Jun 22, 2021

in test/functional/feature_rbf.py:639 in 801496453b outdated

635+        initial_transaction.vout = [CTxOut(1 * COIN, DUMMY_P2WPKH_SCRIPT)]
636+        initial_transaction_hex = txToHex(initial_transaction)
637+        self.nodes[0].sendrawtransaction(initial_transaction_hex, 0)
638+
639+        # Higher fee, higher fee per KB, but the replacement does not provide a relay
640+        # fee conforming to node's `incrementalrelayfee` policy of 1000 sat per KB.

darosior commented at 6:32 pm on June 22, 2021:

The fee and fee per KB are lower though?

ariard commented at 4:29 pm on June 23, 2021:

I think the replacement_transaction output value is CTxOut(int(1 * COIN - 1)) so it has a higher fee/fee per KB, as everything else is constant.

You can verify with the following diff :

0@@ -892,6 +894,8 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
1         // The replacement must pay greater fees than the transactions it
2         // replaces - if we did the bandwidth used by those conflicting
3         // transactions would not be paid for.
4+        LogPrintf("nModifiedFees %d\n", nModifiedFees);
5+        LogPrintf("nConflictingFees %d\n", nConflictingFees);
6         if (nModifiedFees < nConflictingFees)
7         {
8             return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "insufficient fee",

darosior commented at 4:44 pm on June 23, 2021:

Sorry, brainfart.

DrahtBot added the label Tests on Jun 22, 2021

MarcoFalke commented at 6:11 am on June 23, 2021: member

Nice catch. Concept ACK

in test/functional/feature_rbf.py:631 in 801496453b outdated

626@@ -624,6 +627,24 @@ def test_no_inherited_signaling(self):
627         assert_equal(True, self.nodes[0].getmempoolentry(optin_parent_tx['txid'])['bip125-replaceable'])
628         assert_raises_rpc_error(-26, 'txn-mempool-conflict', self.nodes[0].sendrawtransaction, replacement_child_tx["hex"], 0)
629 
630+    def test_replacement_relay_fee(self):
631+        outpoint = make_utxo(self.nodes[0], int(1.1*COIN))

MarcoFalke commented at 6:11 am on June 23, 2021:

any reason to not use miniwallet, like in the previous test?

ariard commented at 4:31 pm on June 23, 2021:

Well I think it makes the test a bit longer because it doesn’t give you an outpoint but an utxo ? Though I took it anyway.

MarcoFalke commented at 4:38 pm on June 23, 2021:

heh, the point is to create the tx with the miniwallet. In that case the test should be three lines or so.

ariard commented at 4:10 pm on June 24, 2021:

Okay I did drop the miniwallet usage as AFAICT its fee_rate argument doesn’t allow the feerate granularity (sat per KvB) required to hit the yet-uncovered branch ? Or at least without modifying node’s default incrementalrelayfee

ariard renamed this:
~~test: Add functional test for replacement penalty check~~
test: Add functional test for replacement relay fee check
on Jun 23, 2021

ariard force-pushed on Jun 23, 2021

in test/functional/feature_rbf.py:650 in 4bcb8d1e54 outdated

645+        replacement_transaction = CTransaction()
646+        replacement_transaction.vin = [CTxIn(COutPoint(int(utxo['txid'], 16), utxo['vout']), nSequence=1)]
647+        replacement_transaction.vout = [CTxOut(1 * COIN, DUMMY_P2WPKH_SCRIPT)]
648+        replacement_transaction.wit.vtxinwit = [CTxInWitness()]
649+        replacement_transaction.wit.vtxinwit[0].scriptWitness.stack = [CScript([OP_TRUE])]
650+        replacement_transaction_hex = txToHex(replacement_transaction)

darosior commented at 4:58 pm on June 23, 2021:

So now this is the very same test as in test_simple_doublespend: https://github.com/bitcoin/bitcoin/blob/7317e14a44c6efc545e6fb9bcedee7174e93a8fa/test/functional/feature_rbf.py#L133-L148

I think (untested) it would be caught in: https://github.com/bitcoin/bitcoin/blob/7317e14a44c6efc545e6fb9bcedee7174e93a8fa/src/validation.cpp#L830-L838 Which is already a covered branch.

You need to keep the ~~lower~~ higher feerate to get to the yet-uncovered branch of: https://github.com/bitcoin/bitcoin/blob/7317e14a44c6efc545e6fb9bcedee7174e93a8fa/src/validation.cpp#L892-L900

And if you modified that because of my previous brainfart i’m sorry :no_mouth:

ariard commented at 4:06 pm on June 24, 2021:

You need to keep the lower feerate to get to the yet-uncovered branch of:

Oh yes effectively, do you mean the higher feerate ?

At 58cdabe6, feerates are the following:

0 node0 2021-06-24T15:51:23.735515Z [httpworker.0] [validation.cpp:831] [PreChecks] newFeeRate 583.33333345 BTC/kvB 
1 node0 2021-06-24T15:51:23.735525Z [httpworker.0] [validation.cpp:832] [PreChecks] oldFeeRate 583.33333333 BTC/kvB

I think it’s a +1 to have different reject_reason :p

DrahtBot commented at 8:11 pm on June 23, 2021: member

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

Conflicts

No conflicts as of last run.

DrahtBot added the label Needs rebase on Jun 24, 2021

ariard force-pushed on Jun 24, 2021

DrahtBot removed the label Needs rebase on Jun 24, 2021

darosior approved

darosior commented at 6:31 pm on June 24, 2021: member

ACK 58cdabe60e – nice!

in test/functional/feature_rbf.py:642 in 58cdabe60e outdated

637+        initial_transaction.vin = [CTxIn(outpoint, nSequence=0)]
638+        initial_transaction.vout = [CTxOut(1 * COIN, DUMMY_P2WPKH_SCRIPT)]
639+        initial_transaction_hex = initial_transaction.serialize().hex()
640+        self.nodes[0].sendrawtransaction(initial_transaction_hex, 0)
641+
642+        # Equal fee, but the replacement does not provide a relay

MarcoFalke commented at 11:55 am on June 25, 2021:

Is is “equal fee” if the fee is increased?

Again, I’d recommend to use MiniWallet, which will simplify this test a lot:

0    def test_replacement_relay_fee(self):
1        wallet = MiniWallet(self.nodes[0])
2        wallet.scan_blocks(start=77, num=1)
3        tx = wallet.send_self_transfer(from_node=self.nodes[0])['tx']
4        tx.vout[0].nValue -= 1
5        assert_raises_rpc_error(-26, "insufficient fee", self.nodes[0].sendrawtransaction, tx.serialize().hex())

ariard commented at 10:59 pm on June 30, 2021:

Ooops, leftover from the multiple updates of this PR with the converation here.

MarcoFalke approved

MarcoFalke commented at 11:57 am on June 25, 2021: member

Left a review comment about a comment.

Also, would be good to use MiniWallet, to that the test doesn’t have to be touched again in the future when someone wants to run policy tests without the wallet. (Lightning doesn’t run on the Bitcoin Core wallet, so I presume the wallet will generally be disabled)

glozow commented at 7:18 pm on June 30, 2021: member

Concept ACK

test: Add test for replacement relay fee check c4ddee64c7

ariard force-pushed on Jun 30, 2021

ariard commented at 10:58 pm on June 30, 2021: member

@MarcoFalke, Fair point, updated at c4ddee6 with MiniWallet and hopefully binding comment, thanks for the code snippet! IMHO, I prefer to use the low-level test framework API to illustrate etchy, confusing case of our mempool logic but I understand for maintainability the usage of the higher API is better.

Note to reviewers, hopefully the following diff should let you exercise the new test coverage, without blurring with other related “insufficient fee” checks:

 0diff --git a/src/validation.cpp b/src/validation.cpp
 1index b48e49a10..5c8008d30 100644
 2--- a/src/validation.cpp
 3+++ b/src/validation.cpp
 4@@ -904,7 +904,7 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
 5         CAmount nDeltaFees = nModifiedFees - nConflictingFees;
 6         if (nDeltaFees < ::incrementalRelayFee.GetFee(nSize))
 7         {
 8-            return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "insufficient fee",
 9+            return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "insufficient relay fee",
10                     strprintf("rejecting replacement %s, not enough additional fees to relay; %s < %s",
11                         hash.ToString(),
12                         FormatMoney(nDeltaFees),
13diff --git a/test/functional/feature_rbf.py b/test/functional/feature_rbf.py
14index ed944274e..9fcdf614a 100755
15--- a/test/functional/feature_rbf.py
16+++ b/test/functional/feature_rbf.py
17@@ -638,7 +638,7 @@ class ReplaceByFeeTest(BitcoinTestFramework):
18         # Higher fee, higher feerate, different txid, but the replacement does not provide a relay
19         # fee conforming to node's `incrementalrelayfee` policy of 1000 sat per KB.
20         tx.vout[0].nValue -= 1
21-        assert_raises_rpc_error(-26, "insufficient fee", self.nodes[0].sendrawtransaction, tx.serialize().hex())
22+        assert_raises_rpc_error(-26, "insufficient relay fee", self.nodes[0].sendrawtransaction, tx.serialize().hex())
23 
24 if __name__ == '__main__':
25     ReplaceByFeeTest().main()

in test/functional/feature_rbf.py:640 in c4ddee64c7

635+        wallet.scan_blocks(start=77, num=1)
636+        tx = wallet.send_self_transfer(from_node=self.nodes[0])['tx']
637+
638+        # Higher fee, higher feerate, different txid, but the replacement does not provide a relay
639+        # fee conforming to node's `incrementalrelayfee` policy of 1000 sat per KB.
640+        tx.vout[0].nValue -= 1

glozow commented at 1:48 pm on July 1, 2021:

(nit) was slightly surprised that 1 was parsed as 1 satoshi and that the tx didn’t need a new signature

0        # Increase the fee by 1 satoshi. Same signature works because it is an anyone-can-spend.
1        tx.vout[0].nValue -= Decimal("0.00000001")

MarcoFalke commented at 4:35 pm on July 1, 2021:

The test wouldn’t pass with the suggestion applied

glozow commented at 1:59 pm on July 1, 2021: member

ACK c4ddee6, one small suggestion if you retouch.

MarcoFalke commented at 4:35 pm on July 1, 2021: member

cr ACK c4ddee64c7f80eee05a95116ef1b1dc8a7601183

MarcoFalke merged this on Jul 1, 2021

MarcoFalke closed this on Jul 1, 2021

MarcoFalke commented at 4:41 pm on July 1, 2021: member

https://marcofalke.github.io/btc_cov/total.coverage/src/validation.cpp.gcov.html#907 should be green in about 24 hours

sidhujag referenced this in commit 1fe713f884 on Jul 3, 2021

gwillen referenced this in commit dbd482d595 on Jun 1, 2022

DrahtBot locked this on Aug 16, 2022

test: Add functional test for replacement relay fee check #22310

Conflicts