test: added test to assert TX decode rpc error on submitpackage rpc

kevkevinpal commented at 2:04 pm on October 23, 2024: contributor

This PR adds coverage for this line https://github.com/bitcoin/bitcoin/blob/master/src/rpc/mempool.cpp#L996

If you run the following you will get no results for submitpackage grep -nri "TX decode failed" ./test/functional

DrahtBot commented at 2:04 pm on October 23, 2024: contributor

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31139.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	instagibbs, tdb3, rkrux, achow101

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:

#31096 (Package validation: accept packages of size 1 by instagibbs)

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 Tests on Oct 23, 2024

in test/functional/rpc_packages.py:381 in 2b5d4ebd87 outdated

376@@ -377,6 +377,10 @@ def test_submitpackage(self):
377         self.log.info("Submitpackage only allows packages of 1 child with its parents")
378         # Chain of 3 transactions has too many generations
379         legacy_pool = node.getrawmempool()
380+
381+        chain_hex_decode_failure = [t["hex"][:-1] + 'X' for t in self.wallet.create_self_transfer_chain(chain_length=2)]

instagibbs commented at 2:16 pm on October 23, 2024:

two empty string might be simpler?

kevkevinpal commented at 10:38 pm on October 23, 2024:

thanks! fair point updated in 4bdb78c

kevkevinpal force-pushed on Oct 23, 2024

in test/functional/rpc_packages.py:382 in 4bdb78cfe7 outdated

376@@ -377,6 +377,9 @@ def test_submitpackage(self):
377         self.log.info("Submitpackage only allows packages of 1 child with its parents")
378         # Chain of 3 transactions has too many generations
379         legacy_pool = node.getrawmempool()
380+
381+        assert_raises_rpc_error(-22, "TX decode failed:", node.submitpackage, ["", ""])
382+

rkrux commented at 5:24 am on October 24, 2024:

While I was going through this test after I checked out the branch in local, I realised that this assertion seems a bit out of context because of the comment on line 378. How about adding this assertion before line 378?

kevkevinpal commented at 11:42 pm on October 24, 2024:

Thanks! I moved it more towards the start of the function in 34c6643

rkrux approved

rkrux commented at 5:25 am on October 24, 2024: none

tACK 4bdb78cfe76414227b749674ed660f4972fb6eda

Make, functional tests successful. Left a small suggestion.

in test/functional/rpc_packages.py:381 in 4bdb78cfe7 outdated

376@@ -377,6 +377,9 @@ def test_submitpackage(self):
377         self.log.info("Submitpackage only allows packages of 1 child with its parents")
378         # Chain of 3 transactions has too many generations
379         legacy_pool = node.getrawmempool()
380+
381+        assert_raises_rpc_error(-22, "TX decode failed:", node.submitpackage, ["", ""])

brunoerg commented at 8:27 am on October 24, 2024:

I don’t think just passing two empty strings is the right way of testing it because:

it only plays with non-hex values and do not exercise the hex parsing.
if the first tx is invalid, the second one has no effect.

kevkevinpal commented at 11:43 pm on October 24, 2024:

Ya that makes sense!

What I did in 34c6643

Is I used a proper hex value and then had the second value malformed so that we can cover more bases

let me know if that looks good to you I can modify it other ways aswell!

instagibbs commented at 11:52 pm on October 24, 2024:

it only plays with non-hex values and do not exercise the hex parsing.

an empty string is valid hex, it’s just an invalid tx, no? If you want to test invalid hex that’s another thing.

if the first tx is invalid, the second one has no effect.

You need two transactions currently for it to get far enough to try and deserialize. I don’t really care which one(s) are valid except for ease of reading.

let me know if that looks good to you I can modify it other ways aswell!

imo having valid hex is a more realistic scenario, but coverage is coverage

instagibbs commented at 11:58 pm on October 24, 2024:

would be good to double-check that the first tx doesn’t make it into the mempool even if it otherwise would

rkrux commented at 9:11 am on October 25, 2024:

Agree, having this assertion would be good because the valid tx would still not make to mempool in this case.

kevkevinpal commented at 3:09 am on October 26, 2024:

ok I added an assert to check for txid in getrawmempool in 1fab1d6

let me know if that looks good to you!

kevkevinpal force-pushed on Oct 24, 2024

in test/functional/rpc_packages.py:385 in 34c6643c2f outdated

380 
381+
382         self.log.info("Submitpackage only allows packages of 1 child with its parents")
383         # Chain of 3 transactions has too many generations
384         legacy_pool = node.getrawmempool()
385+

rkrux commented at 9:11 am on October 25, 2024:

Are these blank lines intentional?

kevkevinpal commented at 3:08 am on October 26, 2024:

I removed them in 1fab1d6

rkrux commented at 9:12 am on October 25, 2024: none

LGTM, will ACK again once @instagibbs’s suggestion is also added.

#31139 (review)

kevkevinpal force-pushed on Oct 26, 2024

DrahtBot added the label CI failed on Oct 26, 2024

DrahtBot commented at 4:50 am on October 26, 2024: contributor

🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/32095511448

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly 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.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

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

rkrux approved

rkrux commented at 10:46 am on October 26, 2024: none

ACK 1fab1d601e40edc7296cf3d60a7e917b0e6493b3

Thanks for incorporating the changes.

in test/functional/rpc_packages.py:377 in 1fab1d601e outdated

368@@ -369,6 +369,13 @@ def test_submit_child_with_parents(self, num_parents, partial_submit):
369     def test_submitpackage(self):
370         node = self.nodes[0]
371 
372+        self.log.info("Submitpackage only allows valid hex inputs")
373+        valid_tx_list = self.wallet.create_self_transfer_chain(chain_length=2)
374+        hex_list = [valid_tx_list[0]["hex"][:-1] + 'X', valid_tx_list[1]["hex"]]
375+        txid_list = [valid_tx_list[0]["txid"], valid_tx_list[1]["txid"]]
376+        assert_raises_rpc_error(-22, "TX decode failed:", node.submitpackage, hex_list)
377+        assert txid_list[0] not in node.getrawmempool()

instagibbs commented at 1:27 pm on October 26, 2024:

0        assert txid_list[0] not in node.getrawmempool()
1        assert txid_list[1] not in node.getrawmempool()

kevkevinpal commented at 7:18 pm on October 27, 2024:

Thanks!

Updated in d7fd766feb2f579bdba0e778bacdeb13103e8282

instagibbs approved

instagibbs commented at 1:30 pm on October 26, 2024: member

ACK 1fab1d601e40edc7296cf3d60a7e917b0e6493b3

with minor suggestion

test: added test to assert TX decode rpc error on submitpackage rpc d7fd766feb

kevkevinpal force-pushed on Oct 27, 2024

DrahtBot removed the label CI failed on Oct 27, 2024

instagibbs approved

instagibbs commented at 5:55 pm on October 31, 2024: member

reACK https://github.com/bitcoin/bitcoin/pull/31139/commits/d7fd766feb2f579bdba0e778bacdeb13103e8282

DrahtBot requested review from rkrux on Oct 31, 2024

in test/functional/rpc_packages.py:376 in d7fd766feb

368@@ -369,6 +369,14 @@ def test_submit_child_with_parents(self, num_parents, partial_submit):
369     def test_submitpackage(self):
370         node = self.nodes[0]
371 
372+        self.log.info("Submitpackage only allows valid hex inputs")
373+        valid_tx_list = self.wallet.create_self_transfer_chain(chain_length=2)
374+        hex_list = [valid_tx_list[0]["hex"][:-1] + 'X', valid_tx_list[1]["hex"]]
375+        txid_list = [valid_tx_list[0]["txid"], valid_tx_list[1]["txid"]]
376+        assert_raises_rpc_error(-22, "TX decode failed:", node.submitpackage, hex_list)

tdb3 commented at 1:24 am on November 1, 2024:

Induced failure with:

0-        hex_list = [valid_tx_list[0]["hex"][:-1] + 'X', valid_tx_list[1]["hex"]]
1+        hex_list = [valid_tx_list[0]["hex"], valid_tx_list[1]["hex"]]

Also verified that the not in node.getrawmempool() asserts also fail, e.g.

 0         self.log.info("Submitpackage only allows valid hex inputs")
 1         valid_tx_list = self.wallet.create_self_transfer_chain(chain_length=2)
 2-        hex_list = [valid_tx_list[0]["hex"][:-1] + 'X', valid_tx_list[1]["hex"]]
 3+        hex_list = [valid_tx_list[0]["hex"], valid_tx_list[1]["hex"]]
 4         txid_list = [valid_tx_list[0]["txid"], valid_tx_list[1]["txid"]]
 5-        assert_raises_rpc_error(-22, "TX decode failed:", node.submitpackage, hex_list)
 6-        assert txid_list[0] not in node.getrawmempool()
 7+        node.submitpackage(hex_list)
 8+        #assert_raises_rpc_error(-22, "TX decode failed:", node.submitpackage, hex_list)
 9+        #assert txid_list[0] not in node.getrawmempool()
10         assert txid_list[1] not in node.getrawmempool()

tdb3 approved

tdb3 commented at 1:25 am on November 1, 2024: contributor

ACK d7fd766feb2f579bdba0e778bacdeb13103e8282

Looks good, thanks. corecheck.dev shows the added coverage for mempool.cpp. Ran a few quick sanity checks.

rkrux approved

rkrux commented at 6:01 pm on November 1, 2024: none

reACK d7fd766feb2f579bdba0e778bacdeb13103e8282

achow101 commented at 9:14 pm on November 1, 2024: member

ACK d7fd766feb2f579bdba0e778bacdeb13103e8282

achow101 merged this on Nov 1, 2024

achow101 closed this on Nov 1, 2024

JoeyVee8666 commented at 0:14 am on November 3, 2024: none

Looks kinda sus…

test: added test to assert TX decode rpc error on submitpackage rpc #31139

Code Coverage & Benchmarks

Reviews

Conflicts