rpc: Allow to ignore specific policy reject reasons #20753

maflcko · 2020-12-23T16:01:23Z

The -acceptnonstdtxn footgun is only enabled for regtest/testnet due to several shortcomings:

It can't be changed without restarting the node
It applies to all txs and all reject reasons

This pull adds a new optional RPC parameter to ignore specific reject reasons for a specific tx.

luke-jr

What's the goal of reinventing this rather than just reopening and reviewing #7533 (which is still maintained)?

luke-jr · 2020-12-23T17:06:09Z

src/bench/block_assemble.cpp

@@ -49,7 +49,7 @@ static void AssembleBlock(benchmark::Bench& bench)

        for (const auto& txr : txs) {
            TxValidationState state;
-            bool ret{::AcceptToMemoryPool(*test_setup.m_node.mempool, state, txr, nullptr /* plTxnReplaced */, false /* bypass_limits */)};
+            bool ret{::AcceptToMemoryPool(*test_setup.m_node.mempool, state, txr, nullptr /* plTxnReplaced */, {} /* ignore_rejects */, false /* bypass_limits */)};


IMO the const empty_ignore_rejects is cleaner.

src/policy/policy.cpp

DrahtBot · 2020-12-23T21:41:35Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

rpc/validation: enable packages through testmempoolaccept #20833 (rpc/validation: enable packages through testmempoolaccept by glozow)
fuzz: Introduce CallOneOf helper to replace switch-case #20828 (fuzz: Introduce CallOneOf helper to replace switch-case by MarcoFalke)
rpc: Remove duplicate name and argNames from CRPCCommand #20012 (rpc: Remove duplicate name and argNames from CRPCCommand by MarcoFalke)
Move SaltedHashers to separate file and add some new ones #19935 (Move SaltedHashers to separate file and add some new ones by achow101)
wallet: Migrate legacy wallets to descriptor wallets #19602 (wallet: Migrate legacy wallets to descriptor wallets by achow101)
Introduce deploymentstatus #19438 (Introduce deploymentstatus by ajtowns)
Refactor: Improve setup_clean_chain semantics #19168 (Refactor: Improve setup_clean_chain semantics by fjahr)

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.

jnewbery · 2020-12-29T10:14:06Z

I think this needs more motivation. Without a strong reason to do this, I tend to agree with the comments on #7533:

"I'm not convinced about the need to ignore based on the exact reason (as that is likely something that's hard to maintain, as reasons change over time)." (RPC: sendrawtransaction: Allow the user to ignore/override specific rejections #7533 (comment))
"This is not going to be maintainable for API clients." (RPC: sendrawtransaction: Allow the user to ignore/override specific rejections #7533 (comment))
"making this too granular makes this unmaintainable as rejection reasons might come and go, or implemented differently, as policy changes. After all, policy is not standardized." (RPC: sendrawtransaction: Allow the user to ignore/override specific rejections #7533 (comment))

maflcko · 2020-12-29T10:25:13Z

The reasons can only change if the server version changes. And the policy reject reasons may already change in all other RPCs (e.g. sendrawtransaction), so I don't see how this pull makes any difference. If you have specific concerns, it would be good to elaborate them with an example, so that it is easier to understand them.

maflcko · 2020-12-29T11:00:34Z

To extend my reply a bit: Currently it is only possible to override the baked-in behavior for some very specific policy checks via command line (e.g. -dustrelayfee) for all network and rpc transactions. The goal of this change is to allow to override policy reject reasons while also giving the user more flexibility than an "on-off" -acceptnonstdtxn setting. The reason that -acceptnonstdtxn is disabled is that it wouldn't be safe to turn on in normal operation for the default user. So I think the flexibility to have a per-tx override without restarting the node is useful. Maybe even outside the primary use case of one-off manual sends.

Simply having broader categories (or even a single category) doesn't solve the problem that policy changes between versions.

luke-jr · 2020-12-29T16:08:58Z

(Concept ACK, will review code in detail later)

sipa · 2020-12-29T19:02:59Z

Concept NACK on exposing this for mainnet. That seems like a huge footgun, even with the warning. What is the use case? We should never treat our own transactions differently than ones coming from the network.

I think it's pretty ugly in any case, but can imagine it's useful for testing.

maflcko · 2020-12-30T08:53:14Z

One use case is to accept a transaction as a miner that has a large fee, but violates a policy limit (e.g. tx-version, dust, ...) and thus has issues to propagate the network and be accepted to the likely-default mempool the miner is running. If someone is using this without thinking, it might lead to issues, just like invalidateblock may lead to issues when used without thinking.

luke-jr · 2020-12-30T17:01:27Z

Where is the footgun? Many policies are based on DoS risk, which doesn't exist if there's a user actively trying to add a transaction.

As for use cases, I know one real-world case where a user accidentally used an uncompressed key for a segwit address. He's figured out his mistake, and in theory he should be able to recover his coins, but he hasn't convinced any miners (last I checked) to modify their software to accept it. (This PR might not be enough to get that one past, but it demonstrates that there are legitimate needs to override policies.)

glozow · 2020-12-31T23:55:05Z

I can imagine a user wanting to look at a transaction just based on consensus rules and not policy... testmempoolaccept will currently tell you exactly which policy a tx is failing; if a user wants to know "does my transaction pass just consensus rules otherwise?" I think ability to toggle on/off might make sense. But if a user is trying to get a nonstandard tx included in a block and can't get a miner to accept it directly, how does this help? Core nodes still wouldn't (and shouldn't) relay it. Unless someone is planning to mine the tx themselves (not the typical user?) it would just be garbage sitting in their local mempool right?

maflcko · 2021-01-01T10:59:04Z

Correct, the transaction won't relay typically. (Unless you know that you are running an old version of Bitcoin Core to relay a tx that future versions of Bitcoin Core consider policy-"valid", but this is probably an edge case). Thus, I think the primary user of this feature is a miner. (And maybe some power users that know exactly what they are doing).

I think ability to toggle on/off might make sense.

I am happy to explore this direction. Though, testmempoolaccept will only tell you the first policy-failure reason, not the second one. So I was thinking that it might be safer to supply all failure reasons explicitly instead of hoping there was only one.

Another option would be for testmempoolaccept to continue and collect all policy-failures on the way and return all of them instead of only the first? This would make an on/off switch safe again, I believe.

luke-jr · 2021-01-01T14:42:26Z

But if a user is trying to get a nonstandard tx included in a block and can't get a miner to accept it directly, how does this help?

This is how a miner would accept it directly.

Another option would be for testmempoolaccept to continue and collect all policy-failures on the way and return all of them instead of only the first?

This seems like a good idea regardless.

glozow · 2021-01-01T19:46:56Z

This is how a miner would accept it directly.

Ah ok. Then, wrt maintainability, it still seems simpler to just set policy on/off entirely?

Another option would be for testmempoolaccept to continue and collect all policy-failures on the way and return all of them instead of only the first? This would make an on/off switch safe again, I believe.

That sounds nice. Although I'd say it's tricky to implement - what we might consider individual policies aren't really mutually exclusive and we usually stop after the first violation because later checks would break / say the same thing but be more expensive. For example too small or scriptSig size. Would we run EvalScript multiple times for the standard flags? I suppose this is where inputs specifying which policies to exclude would make sense, but still requires policy to be well-enumerated/defined.

jnewbery · 2021-01-01T22:47:18Z

Although I'd say it's tricky to implement - what we might consider individual policies aren't really mutually exclusive and we usually stop after the first violation because later checks would break / say the same thing but be more expensive.

Absolutely agree with this. Mempool acceptance logic should be optimized towards simplicity and performance. Making the internal logic more complex so that we can return additional debugging information over an interface that we can't guarantee to be stable doesn't seem like the right decision to me.

The policy rejection reasons should live in ./src/policy, just like the other policy rejection reasons (e.g. IsStandardTx)

This is a pure refactor to pass ignore_rejects from the sendrawtransaction and testmempoolaccept RPCs to policy. Only default-constructed (empty) sets are passed and never used, so this doesn't change behavior.

maflcko · 2021-01-03T18:16:38Z

Although I'd say it's tricky to implement - what we might consider individual policies aren't really mutually exclusive and we usually stop after the first violation because later checks would break / say the same thing but be more expensive.

Absolutely agree with this. Mempool acceptance logic should be optimized towards simplicity and performance. Making the internal logic more complex so that we can return additional debugging information over an interface that we can't guarantee to be stable doesn't seem like the right decision to me.

We already return the failure reason, and it is already not stable, so this wouldn't change anything fundamentally. Also, this doesn't change performance of mempool acceptance logic if not enabled. Yes, it might run checks that aren't mutually exclusive, but what is the problem with that if the user explicitly asks to do that. A user might also rescan the wallet in a loop, even though it has already been rescanned.

DrahtBot · 2021-01-13T09:11:43Z

🐙 This pull request conflicts with the target branch and needs rebase.

_{Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".}

DrahtBot · 2021-12-15T11:21:17Z

There hasn't been much activity lately and the patch still needs rebase. What is the status here?

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

DrahtBot · 2022-03-21T13:06:57Z

There hasn't been much activity lately and the patch still needs rebase. What is the status here?

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

maflcko mentioned this pull request Dec 23, 2020

RPC: sendrawtransaction: Allow the user to ignore/override specific rejections #7533

Closed

luke-jr suggested changes Dec 23, 2020

View reviewed changes

DrahtBot added GUI P2P RPC/REST/ZMQ TX fees and policy Validation labels Dec 23, 2020

maflcko removed GUI P2P Validation labels Dec 23, 2020

maflcko force-pushed the 2012-policyRpcIgnore branch 2 times, most recently from faa81c6 to fa2b393 Compare December 24, 2020 12:38

This was referenced Jan 2, 2021

fuzz: Introduce CallOneOf helper to replace switch-case #20828

Merged

rpc/validation: enable packages through testmempoolaccept #20833

Merged

MarcoFalke added 4 commits January 3, 2021 19:09

refactor: Pass reason out of policy checks

13c15be

The policy rejection reasons should live in ./src/policy, just like the other policy rejection reasons (e.g. IsStandardTx)

refactor: Pass ignore_rejects from RPCs to policy

9d70043

This is a pure refactor to pass ignore_rejects from the sendrawtransaction and testmempoolaccept RPCs to policy. Only default-constructed (empty) sets are passed and never used, so this doesn't change behavior.

rpc: Allow to ignore specific reject reasons

af9b76a

test: Replace acceptnonstdtxn=1 with specific ignore in wallet_basic

2876005

maflcko force-pushed the 2012-policyRpcIgnore branch from fa2b393 to 2876005 Compare January 3, 2021 18:10

DrahtBot added the Needs rebase label Jan 13, 2021

maflcko mentioned this pull request Mar 7, 2021

RPC: minetxlocally #21372

Closed

maflcko mentioned this pull request May 17, 2021

test: add P2PK support to MiniWallet #21945

Merged

luke-jr mentioned this pull request Jul 6, 2022

mempool: Add the bypass_feerate_accuracy option to testmempoolaccept #25532

Closed

maflcko closed this Jul 22, 2022

maflcko deleted the 2012-policyRpcIgnore branch July 22, 2022 13:19

bitcoin locked and limited conversation to collaborators Jul 22, 2023

rpc: Allow to ignore specific policy reject reasons #20753

rpc: Allow to ignore specific policy reject reasons #20753

Uh oh!

Conversation

maflcko commented Dec 23, 2020

Uh oh!

luke-jr left a comment

Choose a reason for hiding this comment

Uh oh!

luke-jr Dec 23, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DrahtBot commented Dec 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

jnewbery commented Dec 29, 2020

Uh oh!

maflcko commented Dec 29, 2020

Uh oh!

maflcko commented Dec 29, 2020

Uh oh!

luke-jr commented Dec 29, 2020

Uh oh!

sipa commented Dec 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Dec 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

luke-jr commented Dec 30, 2020

Uh oh!

glozow commented Dec 31, 2020

Uh oh!

maflcko commented Jan 1, 2021

Uh oh!

luke-jr commented Jan 1, 2021

Uh oh!

glozow commented Jan 1, 2021

Uh oh!

jnewbery commented Jan 1, 2021

Uh oh!

maflcko commented Jan 3, 2021

Uh oh!

DrahtBot commented Jan 13, 2021

Uh oh!

DrahtBot commented Dec 15, 2021

Uh oh!

DrahtBot commented Mar 21, 2022

Uh oh!

Uh oh!

DrahtBot commented Dec 23, 2020 •

edited

Loading

sipa commented Dec 29, 2020 •

edited

Loading

maflcko commented Dec 30, 2020 •

edited

Loading