add RPC (-regtest only) for testing package policy

glozow commented at 4:30 pm on April 12, 2022: member

It would be nice for LN/wallet/app devs to test out package policy, package RBF, etc., but the only interface to do so right now is through unit tests. This PR adds a -regtest only RPC interface so people can test by submitting raw transaction data. It is regtest-only, as it would be unsafe/confusing to create an actual mainnet interface while package relay doesn’t exist.

Note that the functional tests are there to ensure the RPC interface is working properly; they aren’t for testing policy itself. See src/test/txpackage_tests.cpp.

glozow commented at 4:34 pm on April 12, 2022: member

TODOs (probably for future PRs):

Automatically build the package using the child transaction, i.e. when we already have unconfirmed parents in the mempool or mapWallet. I expect to have this before package relay (users shouldn’t be expected to construct packages themselves imo), but just haven’t implemented it yet. LMK if this is something you want now.
Add maxfeerate to allow users to set a maximum package feerate. This just requires adding a TestPackage codepath separate from testmempoolaccept so we can say “just kidding” if the validation succeeds.

glozow added the label RPC/REST/ZMQ on Apr 12, 2022

luke-jr commented at 2:29 am on April 13, 2022: member

It could be unsafe/confusing to create an actual mainnet interface while package relay doesn’t exist.

How so?

However, it’s currently very inconvenient for wallet/application devs to try to test current package policies.

They shouldn’t need to. No assumptions should be made about user package policies…

in src/rpc/mempool.cpp:813 in f091063141 outdated

809@@ -682,6 +810,7 @@ void RegisterMempoolRPCCommands(CRPCTable& t)
810         {"blockchain", &getmempoolinfo},
811         {"blockchain", &getrawmempool},
812         {"blockchain", &savemempool},
813+        {"hidden", &submitrawpackage,},

maflcko commented at 7:26 am on April 13, 2022:

0        {"hidden", &submitrawpackage},

nit?

glozow commented at 5:38 pm on April 13, 2022:

Done

in src/rpc/mempool.cpp:712 in f091063141 outdated

707+        RPCExamples{
708+            HelpExampleCli("testmempoolaccept", "[rawtx1, rawtx2]") +
709+            HelpExampleCli("submitrawtransaction", "[rawtx1, rawtx2]")
710+        },
711+        [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
712+{

maflcko commented at 7:28 am on April 13, 2022:

0        {

nit: I accidentally didn’t fix the whitespace when creating this file, but I think it is fine for new code to use the right amount of spaces.

glozow commented at 5:38 pm on April 13, 2022:

Done

maflcko commented at 7:28 am on April 13, 2022: member

only nits

glozow commented at 3:02 pm on April 13, 2022: member

It could be unsafe/confusing to create an actual mainnet interface while package relay doesn’t exist.

How so?

Until package relay, it’s unreasonable to assume that anything submitted to the mempool this way will propagate. For example, package cpfp allows 0-fee transactions to be accepted to the mempool if they have a high-fee child. But this likely won’t go much further than the user’s mempool, since most nodes will reject 0-fee transactions.

However, it’s currently very inconvenient for wallet/application devs to try to test current package policies.

They shouldn’t need to. No assumptions should be made about user package policies…

The point here is to encourage testing. Even if they aren’t supposed to rely on the interface, the reality is that most nodes run bitcoind and don’t configure their nodes differently, it’s a good idea for wallet/application devs to write a test suite for what their expectations are. I would also prefer to know as early as possible (i.e. PR comment or in RC testing, not catastrophic bug after release) if we’re going to accidentally harm LN tx propagation with a policy restriction, as >$100million of the network is in it.

in src/rpc/mempool.cpp:693 in f091063141 outdated

688+            {
689+                {RPCResult::Type::OBJ_DYN, "tx-results", "transaction results keyed by wtxid",
690+                {
691+                    {RPCResult::Type::OBJ, "xxxx", "transaction wtxid", {
692+                        {RPCResult::Type::STR_HEX, "txid", "The transaction hash in hex"},
693+                        {RPCResult::Type::STR_HEX, "wtxid", "The transaction witness hash in hex"},

ariard commented at 3:41 pm on April 13, 2022:

If you’re already using “wtxid” as a key, maybe not adding it as “value” ?

glozow commented at 4:47 pm on April 13, 2022:

Because it’s possible that the wtxid is different from the key, if there is a same-txid-different-witness tx in the mempool. I could make this more clear maybe?

glozow commented at 5:38 pm on April 13, 2022:

Ok I’ve renamed it to other-wtxid and only use it when communicating that a same-txid-different-witness tx was found. You’re right that the wtxid key was redundant otherwise.

in src/rpc/mempool.cpp:752 in f091063141 outdated

742+    switch(package_result.m_state.GetResult()) {
743+        case PackageValidationResult::PCKG_RESULT_UNSET: break;
744+        case PackageValidationResult::PCKG_POLICY:
745+        case PackageValidationResult::PCKG_MEMPOOL_ERROR:
746+        {
747+            throw JSONRPCTransactionError(TransactionError::INVALID_PACKAGE,

ariard commented at 3:42 pm on April 13, 2022:

Well sounds PackageValidationResult::PCKG_MEMPOOL_ERROR has its equivalent TransactionError::MEMPOOL_ERROR ? Or other reason to use INVALID_PACKAGE ?

glozow commented at 5:37 pm on April 13, 2022:

Good point, TransactionError::MEMPOOL_ERROR is more appropriate here. Switched.

jnewbery commented at 11:31 am on May 30, 2022:

I think you’ve changed the wrong line. Should this be TransactionError::MEMPOOL_ERROR and the case above be TransactionError::INVALID_PACKAGE?

glozow commented at 10:41 pm on May 30, 2022:

Good catch, swapped

in src/rpc/mempool.cpp:679 in f091063141 outdated

669@@ -669,6 +670,133 @@ static RPCHelpMan savemempool()
670     };
671 }
672 
673+static RPCHelpMan submitrawpackage()
674+{
675+    return RPCHelpMan{"submitrawpackage",
676+        "\nSubmit a package of raw transactions (serialized, hex-encoded) to local node (-regtest only). Experimental, API may be unstable.\n"
677+        "\nThe package will be validated and accepted to mempool if it passes consensus and mempool policy rules. Throws a JSONRPCError if anything goes wrong.\n"
678+        "\nWarning: successful submission does not mean the transaction will propagate to other nodes on the network.\n",

ariard commented at 3:49 pm on April 13, 2022:

To inform the package user, you could link back to doc/policy/packages.md somewhere.

glozow commented at 5:37 pm on April 13, 2022:

Added

ariard commented at 3:49 pm on April 13, 2022: member

Minors, otherwise SGTM.

ariard commented at 4:13 pm on April 13, 2022: member

They shouldn’t need to. No assumptions should be made about user package policies…

Why no assumptions should be made about user package policies ? Could you layout the reasoning underpinning that statement ?

I believe to be of a contrary position. I think the default core policy should be designed to be compatible with miner incentives. In the present case, accepting relayed packages should increase your mining fee reward. If we assume that the majority of users are economically rational, they aim for their mempools contents to reflect the state of the next blocks and thus they should relay packages. Under this assumption concerning the base layer, I think an upper layer like LN can expect a policy like package relay to be widely supported.

luke-jr commented at 5:07 pm on April 13, 2022: member

Until package relay, it’s unreasonable to assume that anything submitted to the mempool this way will propagate. For example, package cpfp allows 0-fee transactions to be accepted to the mempool if they have a high-fee child. But this likely won’t go much further than the user’s mempool, since most nodes will reject 0-fee transactions.

Propagation isn’t a guarantee made by sendrawtransaction either, and could very well be in the same situation if node policies don’t align.

Seems like this PR should just be a modification to sendrawtransaction IMO. (If there’s a reason to make it regtest-only, that can still be done without a new RPC method)

Why no assumptions should be made about user package policies ?

Users should ideally decide their own policies. This is especially true when consideration is being made for security purposes.

I think the default core policy should be designed to be compatible with miner incentives.

That’s illogical. Most nodes aren’t miners, and don’t benefit from miner incentives. They create miner incentives, by deciding their own policies (blocks which don’t match the node policy will relay slower).

If we assume that the majority of users are economically rational, they aim for their mempools contents to reflect the state of the next blocks and thus they should relay packages.

There is no economically rational reason for this. Miners have a reason to match node policies, but nodes do not really benefit from matching miners.

Under this assumption concerning the base layer, I think an upper layer like LN can expect a policy like package relay to be widely supported.

It presently isn’t…

glozow force-pushed on Apr 13, 2022

glozow commented at 5:36 pm on April 13, 2022: member

Addressed comments from @MarcoFalke and @ariard. Also renamed s/submitrawpackage/submitpackage because I don’t think we’d ever submit non-raw packages.

in src/rpc/mempool.cpp:676 in 04bb600294 outdated

669@@ -669,6 +670,136 @@ static RPCHelpMan savemempool()
670     };
671 }
672 
673+static RPCHelpMan submitpackage()
674+{
675+    return RPCHelpMan{"submitpackage",
676+        "\nSubmit a package of raw transactions (serialized, hex-encoded) to local node (-regtest only).\n"

luke-jr commented at 5:58 pm on April 13, 2022:

Seems like this PR should just be a modification to sendrawtransaction

glozow commented at 6:06 pm on April 13, 2022:

sendrawtransaction is not regtest-only

luke-jr commented at 6:36 pm on April 13, 2022:

Nor should this be, IMO. Certainly not long-term.

But regardless, the regtest check can be part of the existing RPC too. (I don’t think it’d be the first case of such either IIRC)

maflcko commented at 7:29 am on April 25, 2022:

0        "Submit a package of raw transactions (serialized, hex-encoded) to local node (-regtest only).\n"

I think the one in the beginning is not needed.

glozow commented at 10:39 pm on May 30, 2022:

Removed

jnewbery commented at 4:35 pm on June 1, 2022:

You’ve taken out too many now! The help text description is now just one (very) long line.

glozow commented at 2:47 pm on June 16, 2022:

Wait I’m confused. Are there supposed to be newlines?

maflcko commented at 3:31 pm on June 16, 2022:

Currently yes, newlines are needed within text, but not as a prefix for the description. (My bad, I’ve edited the suggested change above)

maflcko commented at 3:31 pm on June 16, 2022:

We could also do auto-formatting of newlines, but maybe this can be done later

glozow commented at 8:46 am on June 20, 2022:

Ok based on the new suggestion, I’ve added newlines at the end, but not at the beginning. Hopefully that’s correct

ariard approved

ariard commented at 4:30 pm on April 15, 2022: member

Code Review ACK 04bb600

ariard commented at 4:51 pm on April 15, 2022: member

Users should ideally decide their own policies. This is especially true when consideration is being made for security purposes.

Users deciding their own policies doesn’t mean they won’t spontaneously converge towards few common policies per type of bitcoin applications/hosts.

They create miner incentives, by deciding their own policies (blocks which don’t match the node policy will relay slower).

If we define further miner incentives as optimizing block fee reward, i don’t think node policies creates them. Nodes policies (full-rbf, package relay) are just means to achieve the incentives. I’m not sure it’s a sane node policy to reject consensus-valid blocks.

There is no economically rational reason for this. Miners have a reason to match node policies, but nodes do not really benefit from matching miners.

If users decide their own policies with a high degree of arbitrary of miners can expect to match the node policies ? At the contrary, node benefit from their policies matching the miners to improve their visibility of the future blockchain state and block space demand. I think restrictions, which can be motivated for security reasons or host perfomances or whatever, comes with the trade-off of a worsen view of the blockchain.

ghost commented at 7:50 pm on April 15, 2022: none

They shouldn’t need to. No assumptions should be made about user package policies. @luke-jr Agree. Any application assuming mempool policies is vulnerable. Although this RPC could be helpful for testing in general.

glozow requested review from maflcko on Apr 20, 2022

DrahtBot commented at 3:44 am on April 25, 2022: contributor

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#25434 (mempool: Add option to bypass contextual timelocks in testmempoolaccept by w0xlt)
#25038 (BIP125-based 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.

in src/rpc/mempool.cpp:713 in 04bb600294 outdated

708+        RPCExamples{
709+            HelpExampleCli("testmempoolaccept", "[rawtx1, rawtx2]") +
710+            HelpExampleCli("submitrawtransaction", "[rawtx1, rawtx2]")
711+        },
712+        [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
713+    {

maflcko commented at 7:28 am on April 25, 2022:

0        {

nit: spaces missing. I know it is my fault for missing them when I created this file (Is it too late to change now :sweat_smile: ?), but I think new code should use spaces, otherwise it is weird, see also #24408 (review)

glozow commented at 10:41 pm on May 30, 2022:

I assume you meant to indent all of it? Did that in the last push.

maflcko commented at 7:30 am on April 25, 2022: member

left some nits, since you requested a review (not sure if you wanted that kind of review)

michaelfolkson commented at 6:22 am on April 30, 2022: contributor

Seems like this PR should just be a modification to sendrawtransaction IMO. (If there’s a reason to make it regtest-only, that can still be done without a new RPC method)

But regardless, the regtest check can be part of the existing RPC too. (I don’t think it’d be the first case of such either IIRC)

For the benefit of other reviewers this seems on the face of it to be a reasonable counter-suggestion to what this PR is doing that is worth exploring.

The rest of the conversation seems to be a rehashing of the “Lightning security ideally wouldn’t rely on mempool policy” and “One of the motivations for having a mempool is having a set of already validated transactions that the node expects to be mined in an upcoming block”

edit: This is good from the PR author on why it makes sense for the user to care about miner incentives.

in src/rpc/mempool.cpp:677 in 04bb600294 outdated

669@@ -669,6 +670,136 @@ static RPCHelpMan savemempool()
670     };
671 }
672 
673+static RPCHelpMan submitpackage()
674+{
675+    return RPCHelpMan{"submitpackage",
676+        "\nSubmit a package of raw transactions (serialized, hex-encoded) to local node (-regtest only).\n"
677+        "\nThe package will be validated and accepted to mempool if it passes consensus and mempool policy rules. Throws a JSONRPCError if anything goes wrong.\n"

jnewbery commented at 11:15 am on May 30, 2022:

“anything goes wrong” is quite vague. Does it mean “if the transaction or package is rejected for consensus or policy reasons”?

glozow commented at 10:40 pm on May 30, 2022:

Hopefully now clearer

in src/rpc/mempool.cpp:678 in 04bb600294 outdated

669@@ -669,6 +670,136 @@ static RPCHelpMan savemempool()
670     };
671 }
672 
673+static RPCHelpMan submitpackage()
674+{
675+    return RPCHelpMan{"submitpackage",
676+        "\nSubmit a package of raw transactions (serialized, hex-encoded) to local node (-regtest only).\n"
677+        "\nThe package will be validated and accepted to mempool if it passes consensus and mempool policy rules. Throws a JSONRPCError if anything goes wrong.\n"
678+        "\nExperimental. API may be unstable. Refer to docs/policy/packages.md for documentation on package policies.\n"

jnewbery commented at 11:18 am on May 30, 2022:

0        "\nThis RPC is experimental and the interface may be unstable. Refer to doc/policy/packages.md for documentation on package policies.\n"

glozow commented at 10:40 pm on May 30, 2022:

Thanks, taken

in src/rpc/mempool.cpp:740 in 04bb600294 outdated

735+    }
736+
737+    NodeContext& node = EnsureAnyNodeContext(request.context);
738+    CTxMemPool& mempool = EnsureMemPool(node);
739+    CChainState& chainstate = EnsureChainman(node).ActiveChainstate();
740+    const auto package_result = WITH_LOCK(::cs_main, return ProcessNewPackage(chainstate, mempool, txns, /* test_accept */ false));

jnewbery commented at 11:29 am on May 30, 2022:

0    const auto package_result = WITH_LOCK(::cs_main, return ProcessNewPackage(chainstate, mempool, txns, /*test_accept=*/false));

glozow commented at 10:41 pm on May 30, 2022:

Done

in src/rpc/mempool.cpp:839 in 04bb600294 outdated

758+                auto it = package_result.m_tx_results.find(tx->GetWitnessHash());
759+                if (it != package_result.m_tx_results.end() && it->second.m_state.IsInvalid()) {
760+                    throw JSONRPCTransactionError(TransactionError::MEMPOOL_REJECTED,
761+                        strprintf("%s failed: %s", tx->GetHash().ToString(), it->second.m_state.GetRejectReason()));
762+                }
763+            }

jnewbery commented at 11:33 am on May 30, 2022:

Should we either Assume(false) here or raise a JSONRPCTransactionError(TransactionError::MEMPOOL_ERROR)? There shouldn’t be a PackageValidationResult::PCKG_TX without one of the transactions having an invalid status.

glozow commented at 10:41 pm on May 30, 2022:

Good point, added NONFATAL_UNREACHABLE()

in src/rpc/mempool.cpp:696 in 04bb600294 outdated

691+                {
692+                    {RPCResult::Type::OBJ, "xxxx", "transaction wtxid", {
693+                        {RPCResult::Type::STR_HEX, "txid", "The transaction hash in hex"},
694+                        {RPCResult::Type::STR_HEX, "other-wtxid", /*optional=*/ true, "The wtxid of a different transaction with the same txid but different witness found in the mempool. This means the submitted transaction was ignored."},
695+                        {RPCResult::Type::NUM, "vsize", "Virtual transaction size as defined in BIP 141."},
696+                        {RPCResult::Type::OBJ, "fees", "Transaction fees (only present if result=\"valid\")", {

jnewbery commented at 11:36 am on May 30, 2022:

0                        {RPCResult::Type::OBJ, "fees", "Transaction fees", {

(result is always valid if this rpc returns)

glozow commented at 10:40 pm on May 30, 2022:

Ah, leftover from previous iteration, thanks for catching

in test/functional/rpc_packages.py:380 in 04bb600294 outdated

375+        for num_parents in [1, 2, 10, 24]:
376+            self.test_submit_child_with_parents(num_parents)
377+
378+        self.log.info("Submitrawpackage only allows packages of 1 child with its parents")
379+        # Chain of 3 transactions has too many generations
380+        (chain_hex, _) = create_raw_chain(node, self.coins.pop(), self.address, self.privkeys, 3)

jnewbery commented at 11:40 am on May 30, 2022:

No need for this to be a tuple:

0        chain_hex, _ = create_raw_chain(node, self.coins.pop(), self.address, self.privkeys, 3)

glozow commented at 10:42 pm on May 30, 2022:

Done

in test/functional/rpc_packages.py:346 in 04bb600294 outdated

341+        child_hex = create_child_with_parents(node, self.address, self.privkeys, package_txns, values, scripts)
342+        package_hex.append(child_hex)
343+        package_txns.append(tx_from_hex(child_hex))
344+
345+        testres_package = node.testmempoolaccept(rawtxs=package_hex)
346+        submitres_package = node.submitpackage(package=package_hex)

jnewbery commented at 11:46 am on May 30, 2022:

These names seems a bit confusing to me. What do you think about testmempoolaccept_result and submitpackage_result?

glozow commented at 10:42 pm on May 30, 2022:

Done

in test/functional/rpc_packages.py:323 in 04bb600294 outdated

314+        """
315+        for testres_tx in testres_package:
316+            # Grab this result from the submitres
317+            submitres_tx = submitres_package["tx-results"][testres_tx["wtxid"]]
318+            assert_equal(submitres_tx["txid"], testres_tx["txid"])
319+            if "allowed" in testres_tx and testres_tx["allowed"]:

jnewbery commented at 11:46 am on May 30, 2022:

Is this always true?

glozow commented at 10:42 pm on May 30, 2022:

Not if we have partial submissions - added that test case and an explanation comment

in test/functional/rpc_packages.py:368 in 04bb600294 outdated

363+        # submitpackage result should be consistent with testmempoolaccept and getmempoolentry
364+        self.assert_equal_package_results(node, testres_package, submitres_package)
365+
366+        # With no deduplications, the package feerate is just the ancestor feerate of the child.
367+        child_txid = testres_package[-1]["txid"]
368+        if "package-feerate" in submitres_package:

jnewbery commented at 11:46 am on May 30, 2022:

Is this always true?

glozow commented at 10:42 pm on May 30, 2022:

Not if we have partial submissions - added that test case and an explanation comment

jnewbery commented at 11:50 am on May 30, 2022: contributor

Concept and approach ACK. Just some minor style comments.

This could definitely have more extensive testing (eg submitting packages with entries already in the mempool, invalid package topologies, packages containing invalid transactions, etc), but it’s fine to leave that for a follow-up PR. It looks like some of the test code has been written with that flexibility in mind, which I think can be removed if it’s not actually being used yet.

glozow force-pushed on May 30, 2022

glozow commented at 10:53 pm on May 30, 2022: member

Rebased to use NONFATAL_UNREACHABLE

glozow commented at 4:16 am on May 31, 2022: member

Thanks @MarcoFalke @jnewbery, took your comments

WazirXwallet approved

in src/rpc/mempool.cpp:737 in 411cc6eb9f outdated

729@@ -729,6 +730,138 @@ static RPCHelpMan savemempool()
730     };
731 }
732 
733+static RPCHelpMan submitpackage()
734+{
735+    return RPCHelpMan{"submitpackage",
736+        "Submit a package of raw transactions (serialized, hex-encoded) to local node (-regtest only)."
737+        "The package will be validated and accepted to mempool if it passes consensus and mempool policy rules, otherwise throws a JSONRPCError."

jnewbery commented at 4:42 pm on June 1, 2022:

This run on sentence is a bit mangled. How about:

“The package will be validated according to consensus and mempool policy rules. If all transactions pass, they will be accepted to mempool.”

I don’t think you need to mention the JSONRPCError, which is an internal implementation concept. Other RPC methods don’t document error conditions in the help text.

glozow commented at 8:47 am on June 16, 2022:

Done

in src/rpc/mempool.cpp:754 in 411cc6eb9f outdated

749+            {
750+                {RPCResult::Type::OBJ_DYN, "tx-results", "transaction results keyed by wtxid",
751+                {
752+                    {RPCResult::Type::OBJ, "xxxx", "transaction wtxid", {
753+                        {RPCResult::Type::STR_HEX, "txid", "The transaction hash in hex"},
754+                        {RPCResult::Type::STR_HEX, "other-wtxid", /*optional=*/ true, "The wtxid of a different transaction with the same txid but different witness found in the mempool. This means the submitted transaction was ignored."},

jnewbery commented at 4:43 pm on June 1, 2022:

0                        {RPCResult::Type::STR_HEX, "other-wtxid", /*optional=*/true, "The wtxid of a different transaction with the same txid but different witness found in the mempool. This means the submitted transaction was ignored."},

(no space between comment and parameter - same for other instances below)

glozow commented at 8:47 am on June 16, 2022:

Done

in test/functional/rpc_packages.py:309 in 411cc6eb9f outdated

305@@ -306,5 +306,85 @@ def test_rbf(self):
306         }]
307         self.assert_testres_equal(self.independent_txns_hex + [signed_replacement_tx["hex"]], testres_rbf_package)
308 
309+    def assert_equal_package_results(self, node, testres_package, submitres_package):

jnewbery commented at 4:54 pm on June 1, 2022:

What do you think about using the same naming convention here (testmempoolaccept_result and submitpackage_result)?

glozow commented at 8:47 am on June 16, 2022:

Done

in test/functional/rpc_packages.py:369 in 411cc6eb9f outdated

364+            })
365+
366+        # submitpackage result should be consistent with testmempoolaccept and getmempoolentry
367+        self.assert_equal_package_results(node, testmempoolaccept_result, submitpackage_result)
368+
369+        # With no deduplications, the package feerate is just the ancestor feerate of the child.

jnewbery commented at 5:00 pm on June 1, 2022:

This test is done even if this is a partial submission. Is partial submission the same as deduplication? If so, why does this test still pass? Is it because the feerate of every transaction in the package is the same?

glozow commented at 8:47 am on June 16, 2022:

Clarified the comment

in src/rpc/mempool.cpp:739 in 411cc6eb9f outdated

734+{
735+    return RPCHelpMan{"submitpackage",
736+        "Submit a package of raw transactions (serialized, hex-encoded) to local node (-regtest only)."
737+        "The package will be validated and accepted to mempool if it passes consensus and mempool policy rules, otherwise throws a JSONRPCError."
738+        "This RPC is experimental and the interface may be unstable. Refer to doc/policy/packages.md for documentation on package policies."
739+        "Warning: successful submission does not mean the transaction will propagate to other nodes on the network.",

ariard commented at 0:13 am on June 9, 2022:

Unclear comment : this RPC requires package to be at least 2 transactions and this mention a singular transaction. Though I would be even clearer and say something like “for now, the package transactions are going to be relayed individually to the other nodes and might not propagate on the network” ?

glozow commented at 8:47 am on June 16, 2022:

Done

in src/rpc/mempool.cpp:752 in 411cc6eb9f outdated

747+        RPCResult{
748+            RPCResult::Type::OBJ, "", "",
749+            {
750+                {RPCResult::Type::OBJ_DYN, "tx-results", "transaction results keyed by wtxid",
751+                {
752+                    {RPCResult::Type::OBJ, "xxxx", "transaction wtxid", {

ariard commented at 0:14 am on June 9, 2022:

Maybe “wtxid” instead of “xxxx”

glozow commented at 8:47 am on June 16, 2022:

Done

in src/rpc/mempool.cpp:770 in 411cc6eb9f outdated

765+                }},
766+            },
767+        },
768+        RPCExamples{
769+            HelpExampleCli("testmempoolaccept", "[rawtx1, rawtx2]") +
770+            HelpExampleCli("submitrawtransaction", "[rawtx1, rawtx2]")

ariard commented at 0:18 am on June 9, 2022:

There is no such thing as “submitrawtransaction” am I missing something ?

glozow commented at 8:47 am on June 16, 2022:

Sorry yes, sendrawtransaction. Fixed

in src/rpc/mempool.cpp:783 in 411cc6eb9f outdated

776+            }
777+            RPCTypeCheck(request.params, {
778+                UniValue::VARR,
779+            });
780+            const UniValue raw_transactions = request.params[0].get_array();
781+            if (raw_transactions.size() < 1 || raw_transactions.size() > MAX_PACKAGE_COUNT) {

ariard commented at 0:22 am on June 9, 2022:

I don’t know about leaking package policy checks at the RPC-level as now you have duplicated checks (i.e in CheckPackage). If you wanna fail fast, maybe just call the iirc context-free CheckPackage

glozow commented at 8:44 am on June 16, 2022:

I think it’s good to have a max number of transactions on the rpc - this is a “invalid params” error. Could be 500 or something, but 25 is quite natural since it’ll get rejected anyway if it’s more than that. I could make a new number instead of MAX_PACKAGE_COUNT but again, more natural to just use a constant we already have.

in src/rpc/mempool.cpp:761 in 411cc6eb9f outdated

756+                        {RPCResult::Type::OBJ, "fees", "Transaction fees", {
757+                            {RPCResult::Type::STR_AMOUNT, "base", "transaction fee in " + CURRENCY_UNIT},
758+                        }},
759+                    }}
760+                }},
761+                {RPCResult::Type::STR_AMOUNT, "package-feerate", /*optional=*/ true, "package feerate used for feerate checks in " + CURRENCY_UNIT + " per KvB. Excludes transactions which were deduplicated or accepted individually."},

ariard commented at 0:30 am on June 9, 2022:

What do you think about adding a maxfeerate client-level check like in sendrawtransaction ?

glozow commented at 8:42 am on June 16, 2022:

Yeah I think that’s a good idea for the future, see #24836 (comment)

ariard commented at 0:42 am on June 9, 2022: member

For the benefit of other reviewers this seems on the face of it to be a reasonable counter-suggestion to what this PR is doing that is worth exploring.

FWIW, one argument can be made that make it easier to refine a new dedicated interface for future package issuer step by step as we experiment it. Of course, I think we could also extend sendrawtransaction with a regtest flag and package-only arguments. But that starts to be messy and we have more odds to interfere with sendrawtransaction usage, likely one of the most used RPC call across the Bitcoin ecosystem

glozow force-pushed on Jun 16, 2022

fanquake requested review from ariard on Jun 16, 2022

fanquake requested review from instagibbs on Jun 16, 2022

fanquake requested review from jnewbery on Jun 16, 2022

fanquake requested review from t-bast on Jun 16, 2022

in src/rpc/mempool.cpp:739 in d7d8a9b70c outdated

734+{
735+    return RPCHelpMan{"submitpackage",
736+        "Submit a package of raw transactions (serialized, hex-encoded) to local node (-regtest only)."
737+        "The package will be validated according to consensus and mempool policy rules. If all transactions pass, they will be accepted to mempool."
738+        "This RPC is experimental and the interface may be unstable. Refer to doc/policy/packages.md for documentation on package policies."
739+        "Warning: successful submission does not mean the transaction will propagate to other nodes on the network. Until package relay exists, they will each be relayed individually.",

instagibbs commented at 2:13 pm on June 16, 2022:

Maybe be explicit in saying each individual tx will have to pass feefilter checks for propagation?

glozow commented at 8:45 am on June 20, 2022:

Added

instagibbs commented at 2:23 pm on June 16, 2022: member

concept ACK

w0xlt approved

w0xlt commented at 1:07 pm on June 17, 2022: contributor

Tested ACK https://github.com/bitcoin/bitcoin/pull/24836/commits/fbc6b8613eb4e6fd59fa45979aeff71e152c78e7

glozow force-pushed on Jun 20, 2022

glozow commented at 8:44 am on June 20, 2022: member

Last push

fixed the newline stuff in the description #24836 (review)
added a sentence about individual broadcast #24836 (review)
re-added BroadcastTransaction() (not sure if I lost that in a rebase or something)

in src/rpc/mempool.cpp:772 in 9804649894 outdated

767+                }},
768+            },
769+        },
770+        RPCExamples{
771+            HelpExampleCli("testmempoolaccept", "[rawtx1, rawtx2]") +
772+            HelpExampleCli("sendrawtransaction", "[rawtx1, rawtx2]")

t-bast commented at 9:37 am on June 20, 2022:

The second example should use submitpackage, shouldn’t it?

0            HelpExampleCli("testmempoolaccept", "[rawtx1, rawtx2]") +
1            HelpExampleCli("submitpackage", "[rawtx1, rawtx2]")

glozow commented at 1:40 pm on June 23, 2022:

Thanks! Fixed.

t-bast approved

t-bast commented at 11:31 am on June 20, 2022: contributor

Tested ACK https://github.com/bitcoin/bitcoin/pull/24836/commits/980464989449b3b703c9a11e398b2b246dee163a

I modified eclair to use 0-fee commitment transactions and verified that I could use this RPC to submit packages containing a commitment transaction and a child anchor transaction. The RPC is easy to use and the data it returns is helpful.

Seems like this PR should just be a modification to sendrawtransaction IMO.

I think this is a good point given the restrictions made to the topology of allowed packages. Because we’re only considering packages of 1 child with multiple parents, we could use sendrawtransaction with the serialized child transaction and a new argument containing the package parents.

For now, a separate RPC that’s regtest-only (and can thus be replaced / updated without caring too much about backwards-compatibility) is likely a better choice, but when the time comes to expose a mainnet RPC it could make sense to revisit that.

in src/rpc/mempool.cpp:833 in 9804649894 outdated

827+                    NONFATAL_UNREACHABLE();
828+                }
829+            }
830+            for (const auto& tx : txns) {
831+                std::string err_string;
832+                const auto err = BroadcastTransaction(node, tx, err_string, 0, true, true);

ariard commented at 7:30 pm on June 20, 2022:

I think one way to exercise the BroadcastTransaction here could be to extend rpc_packages.py adding a add_p2p_connection() and waiting for an inv to be received with the package transactions txids.

glozow commented at 1:41 pm on June 23, 2022:

Added.

ariard approved

ariard commented at 7:34 pm on June 20, 2022: member

Code Review ACK 9804649

Changes since last review :

RPCHelpMan updates to warn about the lack of package propagation
RPCResult updates around “wtxid” and “other-wtxid”
HelpExampleCli updates from “submitrawtransaction” to “sendrawtransaction”
re-add the BroadcastTransaction (would be nice to test coverage as suggested)
rpc_package.py comments and logs corrections

Edit: I checked my previous reviews, effectively BroadcastTransaction() wasn’t present. From my understanding, without p2p packages, the odds of low-fee parent package propagating are low, so it didn’t really matter if a BroadcastTransaction() call wasn’t present or not. As this RPC is -regtest only and there is already a warning about expected lack of propagation, I think it doesn’t matter if we introduce BroadcastTransaction() in this RPC now or latter.

fanquake requested review from instagibbs on Jun 21, 2022

in src/rpc/mempool.cpp:834 in 421f286615 outdated

828+                }
829+            }
830+            for (const auto& tx : txns) {
831+                std::string err_string;
832+                const auto err = BroadcastTransaction(node, tx, err_string, 0, true, true);
833+                if (err != TransactionError::OK) {

instagibbs commented at 1:06 pm on June 21, 2022:

If we’re hitting this error condition it seems more useful to get back some usable information about the transactions that already were broadcasted in this loop.

glozow commented at 1:47 pm on June 23, 2022:

Note that we only do this after all the transactions have already been submitted successfully, so the error should be rare (in fact, kind of wondering if I should just put NONFATAL_UNREACHABLE). I’ve added the number of transactions broadcasted to the error string though.

instagibbs commented at 1:36 pm on June 30, 2022:

good enough for now

in src/rpc/mempool.cpp:863 in 421f286615 outdated

855+                        for (const auto& ptx : it->second.m_replaced_transactions.value()) {
856+                            replaced_txids.insert(ptx->GetHash());
857+                        }
858+                    }
859+                }
860+                tx_result_map.pushKV(tx->GetWitnessHash().GetHex(), result_inner);

instagibbs commented at 1:10 pm on June 21, 2022:

some places we’re using wtxid for indexing, others txid(replaced txns), is there a guiding principle here?

glozow commented at 12:14 pm on June 23, 2022:

I think wtxids would be better in general. Txids for replaced txns is basically a legacy thing, it’s just using what’s returned in MempoolAcceptResult which is also used in testmempoolaccept. It would be impossible to have multiple replaced transactions with the same txids since they must have all been in mempool at the start. Perhaps one day we can change this for both RPCs?

Wtxids for indexing into the map because (1) it’s more precise and (2) there actually is a possibility of same-txid-different-witness, i.e. see the “other-wtixd” result.

in test/functional/rpc_packages.py:370 in 9804649894 outdated

365+
366+        # submitpackage result should be consistent with testmempoolaccept and getmempoolentry
367+        self.assert_equal_package_results(node, testmempoolaccept_result, submitpackage_result)
368+
369+        # Package feerate is calculated for the remaining transactions after deduplication and
370+        # individual submission.  If only 0 or 1 transaction is left, e.g. because all transactions

instagibbs commented at 1:17 pm on June 21, 2022:

could we assert this condition is the case when “package-feerate” doesn’t exist, rather than have a comment to the effect?

glozow commented at 1:49 pm on June 23, 2022:

Added. Also note that we have more detailed tests for internal logic wrt package feerate in txpackage_tests.cpp

[rpc] add new submitpackage RPC

It could be unsafe/confusing to create an actual mainnet interface while
package relay doesn't exist. However, a regtest-only interface allows
wallet/application devs to test current package policies.

fa076515b0

[functional test] submitrawpackage RPC e866f0d066

glozow force-pushed on Jun 23, 2022

glozow commented at 10:00 am on June 27, 2022: member

Last push:

s/sendrawtransaction/submitpackage #24836 (review)
test that the transactions are broadcast #24836 (review)
test package feerate #24836 (review)
note some transactions may have been broadcast already if BroadcastTransaction() fails #24836 (review)

thanks @t-bast @ariard @instagibbs !

t-bast approved

t-bast commented at 12:40 pm on June 27, 2022: contributor

Tested ACK against eclair https://github.com/bitcoin/bitcoin/pull/24836/commits/e866f0d0666664885d4c15c79bf59cc59975887a

ariard approved

ariard commented at 1:00 am on June 30, 2022: member

Code Review ACK e866f0d0

Changes since last review:

updates sendrawtransaction to submitpackage in HelpExampleCli
yield a JSON rpc error with the number of transaction in package
few functional test updates, notably adding a test_submit_cpfp case

fanquake requested review from instagibbs on Jun 30, 2022

fanquake requested review from darosior on Jun 30, 2022

in test/functional/rpc_packages.py:412 in e866f0d066

407+        # individual submission. Since this package had a 0-fee parent, package feerate must have
408+        # been used and returned.
409+        assert "package-feerate" in submitpackage_result
410+        assert_fee_amount(DEFAULT_FEE, rich_parent_result["vsize"] + child_result["vsize"], submitpackage_result["package-feerate"])
411+
412+        # The node will broadcast each transaction, still abiding by its peer's fee filter

instagibbs commented at 1:57 pm on June 30, 2022:

future work: would be nice if “abiding by its peer’s fee filter” had test coverage rather than a comment to prevent future regressions

instagibbs approved

instagibbs commented at 1:59 pm on June 30, 2022: member

code review ACK e866f0d0666664885d4c15c79bf59cc59975887a

code changes as expected pert: #24836 (comment)

fanquake merged this on Jun 30, 2022

fanquake closed this on Jun 30, 2022

sidhujag referenced this in commit 18543cc736 on Jun 30, 2022

bitcoin locked this on Jun 30, 2023

glozow deleted the branch on Feb 27, 2024

add RPC (-regtest only) for testing package policy #24836

Conflicts