bugfix, Change up submitpackage results to return results for all transactions #28848

instagibbs · 2023-11-10T21:10:11Z

This was prompted by errors being returned that didn't "make any sense" to me, because it would for example return a "fee too low" error, when the "real" error was the child had something invalid, which disallowed CPFP evaluation. Rather than make judgment calls on what error is important(which is currently just return the "first"!), we simply return all errors and let the callers determine what's best.

Added a top level package_msg for quick eye-balling of general success of the package.

This PR also fixes a couple bugs:

Currently we don't actually broadcast a transaction, even if it was entered into our mempool, if a subsequent transaction causes PKG_TX failure.
"other-wtxid" is uncovered by tests, but IIUC was previously required to return "fees" and "vsize" results, but did not. I just make those results optional.

DrahtBot · 2023-11-10T21:10:14Z

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, Sjors, achow101
Stale ACK	darosior

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:

#28950 (RPC: Add maxfeerate and maxburnamount args to submitpackage by instagibbs)
#27591 (rpc: distinguish between vsize and sigop-adjusted mempool vsize 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.

src/rpc/mempool.cpp

Sjors

Mmm, excuse my ignorance, but shouldn't the minimum relay fee be ignored for packages? (Update: I see this is documented: https://github.com/bitcoin/bitcoin/blob/master/doc/policy/packages.md#package-fees-and-feerate, perhaps we could introduce a (test-only) startup argument to allow it? I'll bring this up in #26933)

I created two transactions while offline, wiping the mempool afterwards. To simulate a busy mempool, I set the -minrelaytxfee to 100, giving the parent a little bit less, and the child a little more than the minimum.

src/bitcoind -signet -minrelaytxfee=0.00100
src/bitcoin-cli -signet submitpackage '["02000000000101f7307b92590b1a7d82e878258a62ffaad05d0db510e947c3c331fe7400469cac0000000000fdffffff029d0c05020000000016001453dcf718842726f5236d0c05b1a5d43e12097731a086010000000000160014acdaaab857365e45724557f835694bdbb0c81231014009e5f00ec66eb80f497b771e2eb10cff222c60dc6b1fe865e5371142b1a02af7fbef913b0bc3bd49be679240e3c87bcde58532e53f657f9805352a43da036c965d9a0200", "02000000000101806ee5976b955c424940808f784fe92f746915a4d875dce994f3f72d8c8529aa0000000000fdffffff021027000000000000160014acdaaab857365e45724557f835694bdbb0c812317da7040200000000160014ef9af76b063593e215f4b861b61944b2e27624f60247304402200ac3df7c7e29df622a487ff075b8d224843e06f0848e16b2f02c55f9210eff9c0220133366f78a0191a74efbc348d2269326b6e27e11b885dd5b77a011de8a357fa00121031526c3e43061b7c9259695e75c512f5456b2dcb66ee475ece29b36688b1c35785d9a0200"]'

{
  "package_msg": "transaction failed",
  "tx-results": {
    "43bb567b00ba2d4424441af6702f841f0953e7aadd18adb475a84d5986186199": {
      "txid": "aa29858c2df7f394e9dc75d8a41569742fe94f788f804049425c956b97e56e80",
      "error": "min relay fee not met, 12838 < 13000"
    },
    "dff806dff6456a11d8cd2318615ae5234a4e4d60662731fc39db2857472af4c9": {
      "txid": "930455a47e5339e702cab0c435172a5497ff3af6d160d16224c9e9ad08cd8592",
      "error": "bad-txns-inputs-missingorspent"
    }
  },
  "replaced-transactions": [
  ]
}

Also why is tx-results not an array like the input? (also to make it easier to sanity check that I didn't submit them the wrong way around)

(I can see how, for a script that submits transaction packages, this dictionary makes it easier to get feedback for specific transactions. For manual submission and debugging it's easier to keep the same ordering, but this may not be a common use case.)

Sjors · 2023-11-23T11:42:54Z

src/rpc/mempool.cpp

@@ -822,7 +822,7 @@ static RPCHelpMan submitpackage()
    return RPCHelpMan{"submitpackage",
        "Submit a package of raw transactions (serialized, hex-encoded) to local node.\n"
        "The package must consist of a child with its parents, and none of the parents may depend on one another.\n"
-        "The package will be validated according to consensus and mempool policy rules. If all transactions pass, they will be accepted to mempool.\n"
+        "The package will be validated according to consensus and mempool policy rules. If a transaction passes, it will be accepted to mempool.\n"


Maybe add: "A transaction that's already in the mempool is [ignored, rebroadcast?]."

a -> any ?

taken

Maybe add: "A transaction that's already in the mempool is [ignored, rebroadcast?]."

Do we want to promise that? I'm ambivalent for now.

src/rpc/mempool.cpp

instagibbs · 2023-11-23T14:19:52Z

@Sjors I think you can set -minrelaytxfee=0 to work around it?

Sjors · 2023-11-23T14:56:38Z

@Sjors I think you can set -minrelaytxfee=0 to work around it?

That doesn't simulate a full mempool though.

I wrote a commit that lets my example go through: Sjors@03baacd

However, not sure if it's worth the extra complexity.

Sjors

Anyway, my two main issues are about pre-existing behavior: tx-results was already a dictionary and -minrelaytxfee already behaved that way.

d72aab3 looks good, but I'm (obviously) not familiar enough with mempool stuff to know for sure.

Sjors · 2023-11-23T15:10:51Z

test/functional/mempool_limit.py

@@ -205,7 +206,7 @@ def test_mid_package_eviction(self):

        # Package should be submitted, temporarily exceeding maxmempool, and then evicted.
        with node.assert_debug_log(expected_msgs=["rolling minimum fee bumped"]):
-            assert_raises_rpc_error(-26, "mempool full", node.submitpackage, package_hex)
+            node.submitpackage(package_hex)


Do you still want to check "mempool full" like below?

the check itself is a little weird, so I just double-checked that the package failed. The later checks should check invariants.

instagibbs · 2023-11-27T16:21:50Z

Also why is tx-results not an array like the input? (also to make it easier to sanity check that I didn't submit them the wrong way around)

If they're submitted "backwards" they'll be rejected.

(I can see how, for a script that submits transaction packages, this dictionary makes it easier to get feedback for specific transactions. For manual submission and debugging it's easier to keep the same ordering, but this may not be a common use case.)

Right, I think this is 99%+ of the use-cases. "typical" usage of manual transaction creation should generate transactions with sufficient fees for an attempt at inclusion in a block.

instagibbs · 2023-11-27T17:09:13Z

all feedback is resolved

Sjors · 2023-11-28T13:42:04Z

ACK 4fb3340

darosior

Concept ACK

Currently we don't actually broadcast a transaction, even if it was entered into our mempool, if a subsequent transaction causes PKG_TX failure.

Let's have a test for this?

diff --git a/test/functional/rpc_packages.py b/test/functional/rpc_packages.py
index 6507f6267b..5ae805ccf0 100755
--- a/test/functional/rpc_packages.py
+++ b/test/functional/rpc_packages.py
@@ -340,6 +340,20 @@ class RPCPackagesTest(BitcoinTestFramework):
         assert_raises_rpc_error(-25, "package topology disallowed", node.submitpackage, chain_hex)
         assert_equal(legacy_pool, node.getrawmempool())
 
+        # Create a transaction chain such as only the parent gets accepted (by making the child's
+        # version non-standard). Make sure the parent does get broadcast.
+        self.log.info("If a package is partially submitted, transactions included in mempool get broadcast")
+        peer = node.add_p2p_connection(P2PTxInvStore())
+        txs = self.wallet.create_self_transfer_chain(chain_length=2)
+        hex_partial_acceptance = [txs[0]["hex"], "f" + txs[1]["hex"][1:]]
+        res = node.submitpackage(hex_partial_acceptance)
+        assert_equal(res["package_msg"], "transaction failed")
+        first_wtxid = txs[0]["tx"].getwtxid()
+        assert "error" not in res["tx-results"][first_wtxid]
+        sec_wtxid = next(wtxid for wtxid in res["tx-results"] if wtxid != first_wtxid)
+        assert_equal(res["tx-results"][sec_wtxid]["error"], "version")
+        peer.wait_for_broadcast([first_wtxid])
+
 
 if __name__ == "__main__":
     RPCPackagesTest().main()

The equivalent on master wouldn't pass:

diff --git a/test/functional/rpc_packages.py b/test/functional/rpc_packages.py
index 5644a9f5a8..d64c30bfe0 100755
--- a/test/functional/rpc_packages.py
+++ b/test/functional/rpc_packages.py
@@ -337,6 +337,12 @@ class RPCPackagesTest(BitcoinTestFramework):
         chain_hex = [t["hex"] for t in self.wallet.create_self_transfer_chain(chain_length=25)]
         assert_raises_rpc_error(-25, "package topology disallowed", node.submitpackage, chain_hex)
 
+        peer = node.add_p2p_connection(P2PTxInvStore())
+        txs = self.wallet.create_self_transfer_chain(chain_length=2)
+        hex_partial_acceptance = [txs[0]["hex"], "f" + txs[1]["hex"][1:]]
+        assert_raises_rpc_error(-26, "version", node.submitpackage, hex_partial_acceptance)
+        peer.wait_for_broadcast([txs[0]["tx"].getwtxid()])
+
 
 if __name__ == "__main__":
     RPCPackagesTest().main()

darosior · 2023-11-29T08:26:10Z

test/functional/rpc_packages.py

        # Chain of 3 transactions has too many generations
+        legacy_pool = node.getrawmempool()
        chain_hex = [t["hex"] for t in self.wallet.create_self_transfer_chain(chain_length=25)]


Side note: should it be a chain of 3 transactions?

it means "at least 3", but you're right this is probably too permissive a check. Not touching for now but can if people want me to.

would be nice to fix, maybe in a followup

darosior · 2023-11-29T12:28:45Z

ACK 4fb3340

I considered suggesting to backport this at first, in light of #27609. But i don't think the fix for relaying the parent transaction if the child fails validation matters much for this usecase (mining pool calling submitpackage). At least, not enough to warrant a backport this late i'd say.

instagibbs · 2023-11-29T14:51:15Z

@darosior thanks, took the test with minor modifications to not do hex-surgery :)

darosior

much-less-fun ACK 24c345f

glozow

4fb3340 interface LGTM, some small code errors

src/rpc/mempool.cpp

glozow · 2023-11-29T15:02:41Z

src/rpc/mempool.cpp

@@ -959,6 +977,8 @@ static RPCHelpMan submitpackage()
                            replaced_txids.insert(ptx->GetHash());
                        }
                    }
+                } else {
+                    NONFATAL_UNREACHABLE();


nit/followup: This could instead be a compile-time check if the if/else blocks are converted to a switch statement on result type

glozow · 2023-11-29T15:06:16Z

test/functional/mempool_limit.py

-        assert_raises_rpc_error(-26, "mempool full", node.submitpackage, [tx_parent_just_below["hex"], tx_child_just_above["hex"]])
+        res = node.submitpackage([tx_parent_just_below["hex"], tx_child_just_above["hex"]])
+        for wtxid in [tx_parent_just_below["wtxid"], tx_child_just_above["wtxid"]]:
+            res["tx-results"][wtxid]["error"] == "mempool full"


Are we trying to assert that this is true? Pretty sure this just keeps going if the expression is False

Suggested change

res["tx-results"][wtxid]["error"] == "mempool full"

assert_equal(res["tx-results"][wtxid]["error"], "mempool full")

oops, taken

glozow · 2023-11-29T15:12:46Z

src/rpc/mempool.cpp

-                    // If a PCKG_TX error was returned, there must have been an invalid transaction.
-                    NONFATAL_UNREACHABLE();
+                    // Package-wide error we want to return, but we also want to return individual responses
+                    package_msg = package_result.m_state.GetRejectReason();


nit: can also CHECK_NONFATAL(package_result.m_tx_results.size() == txns.size()); here

actually this is incorrect; PCKG_POLICY is grouped here, which means it's size of txns or 0. Updated the check

glozow · 2023-11-29T15:18:37Z

test/functional/rpc_packages.py

+        assert_equal(res["package_msg"], "transaction failed")
+        first_wtxid = txs[0]["tx"].getwtxid()
+        assert "error" not in res["tx-results"][first_wtxid]
+        sec_wtxid = next(wtxid for wtxid in res["tx-results"] if wtxid != first_wtxid)


can't you just do sec_wtxid = bad_child.getwtxid()?

glozow · 2023-11-29T15:21:57Z

src/rpc/mempool.cpp

+                    result_inner.pushKV("error", "unevaluated");
+                    continue;
+                }
+                CHECK_NONFATAL(it != package_result.m_tx_results.end());


This check is useless? It exits if untrue right before

glozow · 2023-11-29T15:22:34Z

test/functional/rpc_packages.py

        # Chain of 3 transactions has too many generations
+        legacy_pool = node.getrawmempool()
        chain_hex = [t["hex"] for t in self.wallet.create_self_transfer_chain(chain_length=25)]


would be nice to fix, maybe in a followup

instagibbs · 2023-11-29T16:32:30Z

would be nice to fix, maybe in a followup

took the liberty of doing now @glozow

glozow

ACK 2495312

Behavior prior to this commit allows some transactions to enter into the local mempool but not be reported to the user when encountering a PackageValidationResult::PCKG_TX result. This is further compounded with the fact that any transactions submitted to the mempool during this call would also not be relayed to peers, resulting in unexpected behavior. Fix this by, if encountering a package error, reporting all wtxids, along with a new error field, and broadcasting every transaction that was found in the mempool after submission. Note that this also changes fees and vsize to optional, which should also remove an issue with other-wtxid cases.

instagibbs · 2023-11-29T17:57:47Z

fixed CI fuzz rpc corpus issue with relaxed check

instagibbs · 2023-11-30T14:30:11Z

ping @Sjors @darosior @glozow should be ready for re-acks

glozow

utACK f23ba24, thanks for taking the suggestions

Sjors · 2023-12-01T15:12:40Z

Light re-utACK f23ba24

achow101 · 2023-12-01T17:09:58Z

ACK f23ba24

Behavior prior to this commit allows some transactions to enter into the local mempool but not be reported to the user when encountering a PackageValidationResult::PCKG_TX result. This is further compounded with the fact that any transactions submitted to the mempool during this call would also not be relayed to peers, resulting in unexpected behavior. Fix this by, if encountering a package error, reporting all wtxids, along with a new error field, and broadcasting every transaction that was found in the mempool after submission. Note that this also changes fees and vsize to optional, which should also remove an issue with other-wtxid cases. Github-Pull: bitcoin#28848 Rebased-From: b67db52

Github-Pull: bitcoin#28848 Rebased-From: e67a345

Github-Pull: bitcoin#28848 Rebased-From: f23ba24

instagibbs changed the title ~~Change up submitpackage results~~ Change up submitpackage results to return results for all transactions Nov 10, 2023

instagibbs changed the title ~~Change up submitpackage results to return results for all transactions~~ bugfix, Change up submitpackage results to return results for all transactions Nov 10, 2023

instagibbs force-pushed the 2023-11-submitpackage-results branch from 9e0c54e to 25689d0 Compare November 10, 2023 21:14

DrahtBot added CI failed and removed CI failed labels Nov 10, 2023

instagibbs force-pushed the 2023-11-submitpackage-results branch 2 times, most recently from 8dd3531 to e77b702 Compare November 10, 2023 22:02

DrahtBot added CI failed and removed CI failed labels Nov 10, 2023

DrahtBot mentioned this pull request Nov 11, 2023

rpc: distinguish between vsize and sigop-adjusted mempool vsize #27591

Open

glozow reviewed Nov 11, 2023

View reviewed changes

src/rpc/mempool.cpp Show resolved Hide resolved

instagibbs force-pushed the 2023-11-submitpackage-results branch from 5b3d54d to ddd9224 Compare November 13, 2023 15:28

glozow reviewed Nov 13, 2023

View reviewed changes

src/rpc/mempool.cpp Outdated Show resolved Hide resolved

src/rpc/mempool.cpp Show resolved Hide resolved

src/rpc/mempool.cpp Show resolved Hide resolved

src/rpc/mempool.cpp Outdated Show resolved Hide resolved

glozow reviewed Nov 13, 2023

View reviewed changes

src/rpc/mempool.cpp Outdated Show resolved Hide resolved

instagibbs force-pushed the 2023-11-submitpackage-results branch from ddd9224 to d72aab3 Compare November 13, 2023 18:11

DrahtBot added CI failed and removed CI failed labels Nov 13, 2023

glozow mentioned this pull request Nov 14, 2023

Package Relay Project Tracking #27463

Open

49 tasks

Sjors reviewed Nov 23, 2023

View reviewed changes

instagibbs force-pushed the 2023-11-submitpackage-results branch from d72aab3 to 4fb3340 Compare November 27, 2023 16:21

instagibbs mentioned this pull request Nov 27, 2023

RPC: Add maxfeerate and maxburnamount args to submitpackage #28950

Merged

darosior reviewed Nov 29, 2023

View reviewed changes

DrahtBot requested a review from Sjors November 29, 2023 14:54

glozow reviewed Nov 29, 2023

View reviewed changes

instagibbs force-pushed the 2023-11-submitpackage-results branch from 24c345f to 2495312 Compare November 29, 2023 16:32

glozow reviewed Nov 29, 2023

View reviewed changes

DrahtBot requested a review from darosior November 29, 2023 16:39

instagibbs added 3 commits November 29, 2023 12:56

doc: submitpackage vsize results are sigops-adjusted

e67a345

test_submitpackage: only make a chain of 3 txns

f23ba24

instagibbs force-pushed the 2023-11-submitpackage-results branch from 2495312 to f23ba24 Compare November 29, 2023 17:56

DrahtBot added the CI failed label Nov 29, 2023

DrahtBot removed the CI failed label Nov 29, 2023

glozow reviewed Dec 1, 2023

View reviewed changes

DrahtBot removed the request for review from Sjors December 1, 2023 15:12

achow101 merged commit 6b3927f into bitcoin:master Dec 1, 2023
16 checks passed

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Apr 24, 2024

doc: submitpackage vsize results are sigops-adjusted

30e3f5d

Github-Pull: bitcoin#28848 Rebased-From: e67a345

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Apr 24, 2024

test_submitpackage: only make a chain of 3 txns

e6227d4

Github-Pull: bitcoin#28848 Rebased-From: f23ba24

	res["tx-results"][wtxid]["error"] == "mempool full"
	assert_equal(res["tx-results"][wtxid]["error"], "mempool full")

bugfix, Change up submitpackage results to return results for all transactions #28848

bugfix, Change up submitpackage results to return results for all transactions #28848

Conversation

instagibbs commented Nov 10, 2023 • edited

DrahtBot commented Nov 10, 2023 • edited

Code Coverage

Reviews

Conflicts

Sjors left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

instagibbs commented Nov 23, 2023

Sjors commented Nov 23, 2023

Sjors left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

instagibbs commented Nov 27, 2023

instagibbs commented Nov 27, 2023

Sjors commented Nov 28, 2023

darosior left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darosior commented Nov 29, 2023

instagibbs commented Nov 29, 2023

darosior left a comment

Choose a reason for hiding this comment

glozow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

instagibbs commented Nov 29, 2023 • edited

glozow left a comment

Choose a reason for hiding this comment

instagibbs commented Nov 29, 2023

instagibbs commented Nov 30, 2023

glozow left a comment

Choose a reason for hiding this comment

Sjors commented Dec 1, 2023

achow101 commented Dec 1, 2023

instagibbs commented Nov 10, 2023 •

edited

DrahtBot commented Nov 10, 2023 •

edited

Sjors left a comment •

edited

instagibbs commented Nov 29, 2023 •

edited