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

[kevkevinpal]

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]

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.

[rkrux]

tACK 4bdb78c

Make, functional tests successful. Left a small suggestion.

[rkrux]

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]

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

[brunoerg]

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]

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]

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]

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

[rkrux]

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

[kevkevinpal]

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

let me know if that looks good to you!

[rkrux]

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

#31139 (comment)

[DrahtBot]

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

Hints

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]

ACK 1fab1d6

Thanks for incorporating the changes.

[instagibbs]

ACK 1fab1d6

with minor suggestion

[instagibbs]

reACK d7fd766

[tdb3]

ACK d7fd766

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

[tdb3]

Induced failure with:

-        hex_list = [valid_tx_list[0]["hex"][:-1] + 'X', valid_tx_list[1]["hex"]]
+        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.

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

[rkrux]

reACK d7fd766

[achow101]

ACK d7fd766