add RPC (-regtest only) for testing package policy #24836
Conversation
|
TODOs (probably for future PRs):
|
How so?
They shouldn't need to. No assumptions should be made about user package policies... |
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.
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. |
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. |
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
Users should ideally decide their own policies. This is especially true when consideration is being made for security purposes.
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).
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.
It presently isn't... |
f091063 to
04bb600
Compare
|
Addressed comments from @MarcoFalke and @ariard. Also renamed s/submitrawpackage/submitpackage because I don't think we'd ever submit non-raw packages. |
Users deciding their own policies doesn't mean they won't spontaneously converge towards few common policies per type of bitcoin applications/hosts.
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.
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. |
@luke-jr Agree. Any application assuming mempool policies is vulnerable. Although this RPC could be helpful for testing in general. |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
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. |
There was a problem hiding this comment.
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.
62994ee to
411cc6e
Compare
|
Rebased to use |
|
Thanks @MarcoFalke @jnewbery, took your comments |
There was a problem hiding this comment.
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
411cc6e to
fbc6b86
Compare
|
concept ACK |
fbc6b86 to
9804649
Compare
|
Last push
|
There was a problem hiding this comment.
Tested ACK 9804649
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.
There was a problem hiding this comment.
Code Review ACK 9804649
Changes since last review :
RPCHelpManupdates to warn about the lack of package propagationRPCResultupdates around "wtxid" and "other-wtxid"HelpExampleCliupdates from "submitrawtransaction" to "sendrawtransaction"- re-add the
BroadcastTransaction(would be nice to test coverage as suggested) rpc_package.pycomments 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.
| } | ||
| } | ||
| } | ||
| tx_result_map.pushKV(tx->GetWitnessHash().GetHex(), result_inner); |
There was a problem hiding this comment.
some places we're using wtxid for indexing, others txid(replaced txns), is there a guiding principle here?
There was a problem hiding this comment.
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.
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.
9804649 to
e866f0d
Compare
|
Last push:
thanks @t-bast @ariard @instagibbs ! |
There was a problem hiding this comment.
Code Review ACK e866f0d
Changes since last review:
- updates
sendrawtransactiontosubmitpackageinHelpExampleCli - yield a JSON rpc error with the number of transaction in package
- few functional test updates, notably adding a
test_submit_cpfpcase
There was a problem hiding this comment.
code review ACK e866f0d
code changes as expected pert: #24836 (comment)
| assert "package-feerate" in submitpackage_result | ||
| assert_fee_amount(DEFAULT_FEE, rich_parent_result["vsize"] + child_result["vsize"], submitpackage_result["package-feerate"]) | ||
|
|
||
| # The node will broadcast each transaction, still abiding by its peer's fee filter |
There was a problem hiding this comment.
future work: would be nice if "abiding by its peer's fee filter" had test coverage rather than a comment to prevent future regressions
e866f0d [functional test] submitrawpackage RPC (glozow) fa07651 [rpc] add new submitpackage RPC (glozow) Pull request description: 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. ACKs for top commit: t-bast: Tested ACK against eclair bitcoin@e866f0d ariard: Code Review ACK e866f0d instagibbs: code review ACK e866f0d Tree-SHA512: 824a26b10d2240e0fd85e5dd25bf499ee3dd9ba8ef4f522533998fcf767ddded9f001f7a005fe3ab07ec95e696448484e26599803e6034ed2733125c8c376c84
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
-regtestonly 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.