#28984 package rbf followups

instagibbs commented at 6:36 pm on June 17, 2024: member

Some suggested nits/changes from #28984

DrahtBot commented at 6:36 pm on June 17, 2024: 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
ACK	glozow, murchandamus

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

DrahtBot added the label CI failed on Jun 17, 2024

DrahtBot commented at 8:03 pm on June 17, 2024: contributor

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

Debug: https://github.com/bitcoin/bitcoin/runs/26330644984

instagibbs force-pushed on Jun 17, 2024

doc: replace mention of V3 with TRUC de669a883b

instagibbs force-pushed on Jun 17, 2024

instagibbs renamed this:
~~NOMERGE #28984 package rbf followups~~
#28984 package rbf followups
on Jun 17, 2024

instagibbs force-pushed on Jun 18, 2024

DrahtBot removed the label CI failed on Jun 18, 2024

doc: reword package RBF documentation ff4558d441

instagibbs force-pushed on Jun 18, 2024

glozow added the label Refactoring on Jul 2, 2024

in test/functional/mempool_package_rbf.py:171 in 20187f8487 outdated

168@@ -169,22 +169,34 @@ def test_package_rbf_additional_fees(self):
169 
170         self.log.info("Check replacement pays for incremental bandwidth")
171         package_hex3, package_txns3 = self.create_simple_package(coin, parent_fee=DEFAULT_FEE, child_fee=DEFAULT_CHILD_FEE)

glozow commented at 10:46 am on July 4, 2024:

Renaming would help distinguish the variables which are currently reassigned later. Also, since fee doesn’t matter.

0        _, placeholder_txns3 = self.create_simple_package(coin)

instagibbs commented at 5:19 pm on July 9, 2024:

done

in test/functional/mempool_package_rbf.py:182 in 20187f8487 outdated

179-        assert_equal(f"package RBF failed: insufficient anti-DoS fees, rejecting replacement {package_txns3[1].rehash()}, not enough additional fees to relay; 0.00 < 0.00000{sum([tx.get_vsize() for tx in package_txns3])}", pkg_results3["package_msg"])
180-
181+        assert_equal(f"package RBF failed: insufficient anti-DoS fees, rejecting replacement {package_txns3[1].rehash()}, not enough additional fees to relay; {incremental_sats_short} < {incremental_sats_required}", pkg_results3["package_msg"])
182         self.assert_mempool_contents(expected=package_txns1)
183+
184+        package_hex3_1, package_txns3_1 = self.create_simple_package(coin, parent_fee=DEFAULT_FEE, child_fee=DEFAULT_CHILD_FEE + incremental_sats_required)

glozow commented at 10:48 am on July 4, 2024:

nit: variable naming could be more clear, e.g. fail_package_hex3 and success_package_hex3 instead of just numbers

instagibbs commented at 5:19 pm on July 9, 2024:

done

in test/functional/mempool_package_rbf.py:197 in cc1770e6fe outdated

196         pkg_results5 = node.submitpackage(package_hex5)
197         assert 'package RBF failed: package feerate is less than parent feerate' in pkg_results5["package_msg"]
198-
199         self.assert_mempool_contents(expected=package_txns4)
200+
201+        package_hex5_1, package_txns5_1 = self.create_simple_package(coin, parent_fee=DEFAULT_CHILD_FEE, child_fee=DEFAULT_CHILD_FEE + Decimal("0.00000001"))

glozow commented at 10:50 am on July 4, 2024:

nit: out of scope for this PR, but I think it’s high time we create a constant that represents 1 satoshi as a Decimal. Would save a lot of time counting zeroes…

in src/test/util/txmempool.cpp:81 in adcfd69e44 outdated

78 
79         // Replacements can't happen for subpackages larger than 2
80         if (!atmp_result.m_replaced_transactions.empty() &&
81             atmp_result.m_wtxids_fee_calculations.has_value() && atmp_result.m_wtxids_fee_calculations.value().size() > 2) {
82-             return strprintf("tx %s was part of a too-large package RBF subpackage",
83+             return strprintf("RBF subpackage with tx %s was too large",

glozow commented at 10:56 am on July 4, 2024:

I think adcfd69e443475f941c0b0f9d1a0246801ab5dad can be dropped.

See #28984 (review). The error strings are correct to be in past tense instead of conditional.

instagibbs commented at 5:19 pm on July 9, 2024:

removed

test package rbf boundary conditions more closely ad7f1f697f

package rbf: cpfp structure requires package > parent feerate 3f00aae140

instagibbs force-pushed on Jul 9, 2024

glozow commented at 2:20 pm on July 11, 2024: member

ACK 3f00aae1409

glozow commented at 2:42 pm on July 11, 2024: member

cc @murchandamus, incorporates some of your test improvement suggestions #28984 (review)

murchandamus commented at 3:11 pm on July 11, 2024: contributor

ACK 3f00aae14092ca076cff7f9ddf6155c601979fcd

Thanks for taking time to follow-up on my comments.

fanquake merged this on Jul 12, 2024

fanquake closed this on Jul 12, 2024

#28984 package rbf followups #30295

Code Coverage

Reviews