add unit tests for RBF rules in isolation #25674

glozow · 2022-07-22T11:33:38Z

Test each RBF rule more thoroughly and in isolation so we're not relying on things like overall mempool acceptance logic, ordering of mempool checks, RPC results, etc.

RBF was pretty recently refactored out, so there isn't much unit test coverage. From https://marcofalke.github.io/btc_cov/test_bitcoin.coverage/src/policy/rbf.cpp.gcov.html:

jonatack · 2022-07-22T11:40:49Z

Concept ACK and code looks good on first read.

test/rbf_tests.cpp:47:14: error: calling function 'addUnchecked' requires holding mutex 'pool.cs' exclusively [-Werror,-Wthread-safety-analysis]
        pool.addUnchecked(entry.FromTx(next_tx));
             ^
test/rbf_tests.cpp:47:14: error: calling function 'addUnchecked' requires holding mutex 'cs_main' exclusively [-Werror,-Wthread-safety-analysis]

instagibbs

crACK 645c593

Just a few improvements suggested

src/test/rbf_tests.cpp

DrahtBot · 2022-07-23T01:14:31Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#25038 (policy: nVersion=3 and Package RBF by glozow)

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.

instagibbs · 2022-07-25T14:19:18Z

reACK 433411a

src/test/rbf_tests.cpp

darosior

Concept ACK

src/test/rbf_tests.cpp

jamesob · 2022-07-26T11:47:26Z

Concept ACK, will review soon

darosior

ACK 520657a

jonatack

Still looking at this but seeing tests green with very different values for low_fee and normal_fee and high_fee. Only when low_fee > high_fee do I start to see some red (which may make sense given the RBF rules, but then maybe use test values that highlight the minimum necessary value deltas). The tests pass with these values, for instance:

-    const CAmount low_fee{100};
-    const CAmount normal_fee{10000};
-    const CAmount high_fee{1 * COIN};
+    const CAmount low_fee{1000000};
+    const CAmount normal_fee{100};
+    const CAmount high_fee{1000000};

Various minor comments follow, feel free to pick/choose/ignore.

jonatack · 2022-07-27T14:13:17Z

src/Makefile.test.include

@@ -117,6 +117,7 @@ BITCOIN_TESTS =\
  test/prevector_tests.cpp \
  test/raii_event_tests.cpp \
  test/random_tests.cpp \
+  test/rbf_tests.cpp \


Optional, feel free to ignore, maybe add this file to the RUN_TIDY include-what-you-use list in ci/test/06_script_b.sh and update the headers with any relevant suggestions in the tidy CI output.

src/test/rbf_tests.cpp

t-bast

ACK 520657a

glozow · 2022-07-28T10:33:06Z

Still looking at this but seeing tests green with very different values for low_fee and normal_fee and high_fee.

The fees should now be 10x apart each, so hopefully less arbitrary.

Also, apologies for the confusion, I should have mentioned that I pulled this out of #25038 which includes a new check for ancestor feerate. The low-high fees are essentially CPFPs and make a significant difference there, but are largely ignored in the current rules (hence all the "notice how ancestor feerate is not checked here" test cases in this PR). There's probably still room to make the values tighter, but hopefully this background helps.

Test each component of the RBF policy in isolation. Unlike the RBF functional tests, these do not rely on things like RPC results, mempool submission, etc.

w0xlt

ACK c320cdd

instagibbs · 2022-07-28T15:20:46Z

reACK c320cdd

jonatack · 2022-07-28T15:25:36Z

Thanks for the context.

ACK c320cdd

(modulo I haven't yet looked at #25038)

glozow · 2022-07-28T16:22:29Z

This has 3 ACKs (thanks @w0xlt, @instagibbs, @jonatack) and the previous state had 2 other ACKs (thanks @darosior, @t-bast). This PR is just adding test coverage and doesn't conflict with anything other than my draft PR it's pulled out from. As such, it seems there is sufficient review to merge even though I am the author.

c320cdd [unit tests] individual RBF Rules in isolation (glozow) Pull request description: Test each RBF rule more thoroughly and in isolation so we're not relying on things like overall mempool acceptance logic, ordering of mempool checks, RPC results, etc. RBF was pretty recently refactored out, so there isn't much unit test coverage. From https://marcofalke.github.io/btc_cov/test_bitcoin.coverage/src/policy/rbf.cpp.gcov.html: ![image](https://user-images.githubusercontent.com/25183001/180783280-6777f4b4-ef95-462a-b414-1a9e268836a6.png) ACKs for top commit: instagibbs: reACK bitcoin@c320cdd jonatack: ACK c320cdd w0xlt: ACK bitcoin@c320cdd Tree-SHA512: dab555214496255801b9ea92b7bf708bba1ff23edf055c85e29be5eab7d7a863440ee19588aacdce54b2c03feaa4b5963eb159ed89473560bd228737cbfec160

ghost · 2022-08-11T13:26:33Z

This PR is just adding test coverage and doesn't conflict with anything other than my draft PR it's pulled out from. As such, it seems there is sufficient review to merge even though I am the author.

It seems review was insufficient. Adding wrong or low quality test coverage could lead to things not being tested but assumed to be tested.

49db42c [test] make tx6 child of tx5, not tx3, in rbf_tests (glozow) Pull request description: A small overlooked oopsie from #25674. There is no effect on the test results because tx3 and tx5 pay the same fee, but this was the intended configuration, as the comment suggests. ACKs for top commit: instagibbs: ACK 49db42c darosior: Github diff ACK 49db42c. Should have catched this. :/ Tree-SHA512: 2f54337ac3edc38707115cde5b466a85b8a6ac0a0a507effa0e9fecb12c9be196ecd1b16702bc23ba617cfb6a3b5db27d3b71616b3c2dadb186c699c4609831e

49db42c [test] make tx6 child of tx5, not tx3, in rbf_tests (glozow) Pull request description: A small overlooked oopsie from bitcoin#25674. There is no effect on the test results because tx3 and tx5 pay the same fee, but this was the intended configuration, as the comment suggests. ACKs for top commit: instagibbs: ACK bitcoin@49db42c darosior: Github diff ACK 49db42c. Should have catched this. :/ Tree-SHA512: 2f54337ac3edc38707115cde5b466a85b8a6ac0a0a507effa0e9fecb12c9be196ecd1b16702bc23ba617cfb6a3b5db27d3b71616b3c2dadb186c699c4609831e

glozow added the Tests label Jul 22, 2022

glozow force-pushed the 2022-07-rbf-unit-tests branch from b707bfe to 645c593 Compare July 22, 2022 16:26

fanquake requested review from instagibbs, darosior, ariard and t-bast July 22, 2022 16:55

instagibbs approved these changes Jul 22, 2022

View reviewed changes

src/test/rbf_tests.cpp Outdated Show resolved Hide resolved

src/test/rbf_tests.cpp Show resolved Hide resolved

src/test/rbf_tests.cpp Outdated Show resolved Hide resolved

src/test/rbf_tests.cpp Show resolved Hide resolved

src/test/rbf_tests.cpp Outdated Show resolved Hide resolved

DrahtBot mentioned this pull request Jul 23, 2022

policy: nVersion=3 and Package RBF #25038

Closed

glozow force-pushed the 2022-07-rbf-unit-tests branch from 645c593 to 433411a Compare July 25, 2022 07:48

fanquake requested a review from instagibbs July 25, 2022 14:16

ariard reviewed Jul 25, 2022

View reviewed changes

darosior reviewed Jul 26, 2022

View reviewed changes

src/test/rbf_tests.cpp Show resolved Hide resolved

src/test/rbf_tests.cpp Show resolved Hide resolved

src/test/rbf_tests.cpp Show resolved Hide resolved

glozow force-pushed the 2022-07-rbf-unit-tests branch from 433411a to 520657a Compare July 26, 2022 10:25

darosior approved these changes Jul 27, 2022

View reviewed changes

jonatack reviewed Jul 27, 2022

View reviewed changes

t-bast approved these changes Jul 28, 2022

View reviewed changes

glozow force-pushed the 2022-07-rbf-unit-tests branch from 520657a to 2a29c84 Compare July 28, 2022 10:51

[unit tests] individual RBF Rules in isolation

c320cdd

Test each component of the RBF policy in isolation. Unlike the RBF functional tests, these do not rely on things like RPC results, mempool submission, etc.

glozow force-pushed the 2022-07-rbf-unit-tests branch from 2a29c84 to c320cdd Compare July 28, 2022 11:05

w0xlt approved these changes Jul 28, 2022

View reviewed changes

glozow merged commit 41205bf into bitcoin:master Jul 28, 2022

glozow deleted the 2022-07-rbf-unit-tests branch July 28, 2022 16:23

glozow mentioned this pull request Aug 11, 2022

[test] make tx6 child of tx5, not tx3, in rbf_tests #25820

Merged

glozow mentioned this pull request Sep 26, 2022

test: Extend test coverage of BIP125 and document confusing/inconsistent behavior #22867

Closed

bitcoin locked and limited conversation to collaborators Aug 11, 2023

add unit tests for RBF rules in isolation #25674

add unit tests for RBF rules in isolation #25674

Uh oh!

Conversation

glozow commented Jul 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonatack commented Jul 22, 2022

Uh oh!

instagibbs left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DrahtBot commented Jul 23, 2022

Conflicts

Uh oh!

instagibbs commented Jul 25, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

darosior left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jamesob commented Jul 26, 2022

Uh oh!

darosior left a comment

Choose a reason for hiding this comment

Uh oh!

jonatack left a comment • 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!

jonatack Jul 27, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

t-bast left a comment

Choose a reason for hiding this comment

Uh oh!

glozow commented Jul 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

w0xlt left a comment

Choose a reason for hiding this comment

Uh oh!

instagibbs commented Jul 28, 2022

Uh oh!

jonatack commented Jul 28, 2022

Uh oh!

glozow commented Jul 28, 2022

Uh oh!

ghost commented Aug 11, 2022

Uh oh!

Uh oh!

glozow commented Jul 22, 2022 •

edited

Loading

jonatack left a comment •

edited

Loading

glozow commented Jul 28, 2022 •

edited

Loading