test: Add functional test for replacement relay fee check by ariard · Pull Request #22310 · bitcoin/bitcoin

ariard · 2021-06-22T17:21:31Z

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.

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

maflcko · 2021-06-22T17:31:28Z

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".

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

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.

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.

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)

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

maflcko · 2021-06-22T18:23:24Z

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

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

darosior · 2021-06-22T18:32:05Z

The fee and fee per KB are lower though?

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 :

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

Sorry, brainfart.

maflcko · 2021-06-23T06:11:00Z

Nice catch. Concept ACK

maflcko · 2021-06-23T06:11:35Z

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

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.

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

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

darosior · 2021-06-23T16:58:30Z

So now this is the very same test as in test_simple_doublespend:

bitcoin/test/functional/feature_rbf.py

Lines 133 to 148 in 7317e14

tx1a = CTransaction()

tx1a.vin = [CTxIn(tx0_outpoint, nSequence=0)]

tx1a.vout = [CTxOut(1 * COIN, DUMMY_P2WPKH_SCRIPT)]

tx1a_hex = txToHex(tx1a)

tx1a_txid = self.nodes[0].sendrawtransaction(tx1a_hex, 0)

self.sync_all()

# Should fail because we haven't changed the fee

tx1b = CTransaction()

tx1b.vin = [CTxIn(tx0_outpoint, nSequence=0)]

tx1b.vout = [CTxOut(1 * COIN, DUMMY_2_P2WPKH_SCRIPT)]

tx1b_hex = txToHex(tx1b)

# This will raise an exception due to insufficient fee

assert_raises_rpc_error(-26, "insufficient fee", self.nodes[0].sendrawtransaction, tx1b_hex, 0)

I think (untested) it would be caught in:

bitcoin/src/validation.cpp

Lines 830 to 838 in 7317e14

CFeeRate oldFeeRate(mi->GetModifiedFee(), mi->GetTxSize());

if (newFeeRate <= oldFeeRate)

{

return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "insufficient fee",

strprintf("rejecting replacement %s; new feerate %s <= old feerate %s",

hash.ToString(),

newFeeRate.ToString(),

oldFeeRate.ToString()));

}

Which is already a covered branch.

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

bitcoin/src/validation.cpp

Lines 892 to 900 in 7317e14

// The replacement must pay greater fees than the transactions it

// replaces - if we did the bandwidth used by those conflicting

// transactions would not be paid for.

if (nModifiedFees < nConflictingFees)

{

return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "insufficient fee",

strprintf("rejecting replacement %s, less fees than conflicting txs; %s < %s",

hash.ToString(), FormatMoney(nModifiedFees), FormatMoney(nConflictingFees)));

}

And if you modified that because of my previous brainfart i'm sorry 😶

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:

node0 2021-06-24T15:51:23.735515Z [httpworker.0] [validation.cpp:831] [PreChecks] newFeeRate 583.33333345 BTC/kvB 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 · 2021-06-23T20:11:58Z

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

Conflicts

No conflicts as of last run.

darosior

ACK 58cdabe60e -- nice!

maflcko

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)

maflcko · 2021-06-25T11:55:45Z

Is is "equal fee" if the fee is increased?

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

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

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

glozow

Concept ACK

ariard · 2021-06-30T22:58:48Z

@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:

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

glozow

ACK c4ddee6, one small suggestion if you retouch.

glozow · 2021-07-01T13:48:43Z

+
+        # Higher fee, higher feerate, different txid, but the replacement does not provide a relay
+        # fee conforming to node's `incrementalrelayfee` policy of 1000 sat per KB.
+        tx.vout[0].nValue -= 1


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

Suggested change

tx.vout[0].nValue -= 1

# Increase the fee by 1 satoshi. Same signature works because it is an anyone-can-spend.

tx.vout[0].nValue -= Decimal("0.00000001")

The test wouldn't pass with the suggestion applied

maflcko · 2021-07-01T16:35:59Z

cr ACK c4ddee6

maflcko · 2021-07-01T16:41:22Z

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

…fee check c4ddee6 test: Add test for replacement relay fee check (Antoine Riard) Pull request description: 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. ``` // Finally in addition to paying more fees than the conflicts the // new transaction must pay for its own bandwidth. CAmount nDeltaFees = nModifiedFees - nConflictingFees; if (nDeltaFees < ::incrementalRelayFee.GetFee(nSize)) { return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "insufficient fee", strprintf("rejecting replacement %s, not enough additional fees to relay; %s < %s", hash.ToString(), FormatMoney(nDeltaFees), FormatMoney(::incrementalRelayFee.GetFee(nSize)))); } ``` ACKs for top commit: MarcoFalke: cr ACK c4ddee6 glozow: ACK c4ddee6, one small suggestion if you retouch. Tree-SHA512: 7c5d1065db6e6fe57a9f083bf051a7a55eb9892de3a2888679d4a6853491608c93b6e35887ef383a9988d14713fa13a0b1d6134b7354af5fd54765f0d4e98568

maflcko reviewed Jun 22, 2021

View reviewed changes

ariard force-pushed the 2021-06-add-rbf5-test branch from d19c7cc to cdb84f1 Compare June 22, 2021 17:42

DrahtBot added the Needs rebase label Jun 22, 2021

ariard force-pushed the 2021-06-add-rbf5-test branch 4 times, most recently from 8854ad6 to 8014964 Compare June 22, 2021 18:18

maflcko reviewed Jun 22, 2021

View reviewed changes

DrahtBot removed the Needs rebase label Jun 22, 2021

darosior reviewed Jun 22, 2021

View reviewed changes

DrahtBot added the Tests label Jun 22, 2021

maflcko reviewed Jun 23, 2021

View reviewed changes

ariard changed the title ~~test: Add functional test for replacement penalty check~~ test: Add functional test for replacement relay fee check Jun 23, 2021

ariard force-pushed the 2021-06-add-rbf5-test branch from 8014964 to 4bcb8d1 Compare June 23, 2021 16:31

darosior reviewed Jun 23, 2021

View reviewed changes

DrahtBot mentioned this pull request Jun 23, 2021

test: refactor: various (de)serialization helpers cleanups/improvements #22257

Merged

DrahtBot added the Needs rebase label Jun 24, 2021

ariard force-pushed the 2021-06-add-rbf5-test branch from 4bcb8d1 to 58cdabe Compare June 24, 2021 16:02

DrahtBot removed the Needs rebase label Jun 24, 2021

darosior approved these changes Jun 24, 2021

View reviewed changes

maflcko approved these changes Jun 25, 2021

View reviewed changes

glozow reviewed Jun 30, 2021

View reviewed changes

test: Add test for replacement relay fee check

c4ddee6

ariard force-pushed the 2021-06-add-rbf5-test branch from 58cdabe to c4ddee6 Compare June 30, 2021 22:53

glozow reviewed Jul 1, 2021

View reviewed changes

maflcko merged commit a926d6d into bitcoin:master Jul 1, 2021

gwillen pushed a commit to ElementsProject/elements that referenced this pull request Jun 1, 2022

Merge a926d6d into merged_master (Bitcoin PR bitcoin/bitcoin#22310)

dbd482d

bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022

	tx1a = CTransaction()
	tx1a.vin = [CTxIn(tx0_outpoint, nSequence=0)]
	tx1a.vout = [CTxOut(1 * COIN, DUMMY_P2WPKH_SCRIPT)]
	tx1a_hex = txToHex(tx1a)
	tx1a_txid = self.nodes[0].sendrawtransaction(tx1a_hex, 0)

	self.sync_all()

	# Should fail because we haven't changed the fee
	tx1b = CTransaction()
	tx1b.vin = [CTxIn(tx0_outpoint, nSequence=0)]
	tx1b.vout = [CTxOut(1 * COIN, DUMMY_2_P2WPKH_SCRIPT)]
	tx1b_hex = txToHex(tx1b)

	# This will raise an exception due to insufficient fee
	assert_raises_rpc_error(-26, "insufficient fee", self.nodes[0].sendrawtransaction, tx1b_hex, 0)

	CFeeRate oldFeeRate(mi->GetModifiedFee(), mi->GetTxSize());
	if (newFeeRate <= oldFeeRate)
	{
	return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "insufficient fee",
	strprintf("rejecting replacement %s; new feerate %s <= old feerate %s",
	hash.ToString(),
	newFeeRate.ToString(),
	oldFeeRate.ToString()));
	}

	// The replacement must pay greater fees than the transactions it
	// replaces - if we did the bandwidth used by those conflicting
	// transactions would not be paid for.
	if (nModifiedFees < nConflictingFees)
	{
	return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "insufficient fee",
	strprintf("rejecting replacement %s, less fees than conflicting txs; %s < %s",
	hash.ToString(), FormatMoney(nModifiedFees), FormatMoney(nConflictingFees)));
	}

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

Conversation

ariard commented Jun 22, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ariard Jun 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

maflcko commented Jun 23, 2021

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!

darosior Jun 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Jun 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

darosior left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko 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!

glozow left a comment

Choose a reason for hiding this comment

Uh oh!

ariard commented Jun 30, 2021

Uh oh!

glozow 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!

maflcko commented Jul 1, 2021

Uh oh!

maflcko commented Jul 1, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

ariard Jun 22, 2021 •

edited

Loading

darosior Jun 23, 2021 •

edited

Loading

DrahtBot commented Jun 23, 2021 •

edited

Loading