rpc: Allow testmempoolaccept to test unsigned transactions #14939

pull lontivero wants to merge 2 commits into bitcoin:master from lontivero:TestAcceptanceForUnsignedTransactions changing 5 files +55 −12

lontivero commented at 5:35 PM on December 12, 2018: contributor

testmempoolaccept RPC is useful for checking if signed raw transaction can be accepted into the current transaction pool, however there is no available mechanism from checking whether an still unsigned transaction could be accepted or not, except for the fact that is not signed yet.

This PR adds an optional parameter that provides a way for the caller to specify the script evaluation should is skipped. Services that build transactions collectively require to perform several rpc calls in order to verify that built transaction could be accepted before requesting the parts to sign it. This flag could simplify that process.

Fixes #14859
fanquake added the label RPC/REST/ZMQ on Dec 12, 2018

in src/validation.h:309 in a6fa6125d1 outdated

 305 | @@ -306,7 +306,7 @@ void PruneBlockFilesManual(int nManualPruneHeight);
 306 |   * plTxnReplaced will be appended to with all transactions replaced from mempool **/
 307 |  bool AcceptToMemoryPool(CTxMemPool& pool, CValidationState &state, const CTransactionRef &tx,
 308 |                          bool* pfMissingInputs, std::list<CTransactionRef>* plTxnReplaced,
 309 | -                        bool bypass_limits, const CAmount nAbsurdFee, bool test_accept=false) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
 310 | +                        bool bypass_limits, const CAmount nAbsurdFee, bool test_accept=false, bool test_accept_unsigned=false) EXCLUSIVE_LOCKS_REQUIRED(cs_main);

MarcoFalke commented at 5:40 PM on December 12, 2018:

This already has way too many positional arguments. I'd prefer to make test_accept a enum class to avoid passing in single bits that are only supposed to be set exclusively?

lontivero commented at 4:45 AM on December 13, 2018:

I've replaced the test_accept by an enum class called MemPoolValidationScope. Is this better? Any suggestion is very welcome.

MarcoFalke commented at 5:42 PM on December 12, 2018: member

Fixes #14859?
lontivero commented at 5:44 PM on December 12, 2018: contributor

Yes, it fixes #14859.
DrahtBot commented at 9:29 PM on December 12, 2018: member


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



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #14987 (RPCHelpMan: Pass through Result and Examples by MarcoFalke)
- #14918 (RPCHelpMan: Check default values are given at compile-time by MarcoFalke)
- #11652 (Add missing locks: validation.cpp + related by practicalswift)
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.
nopara73 approved
nopara73 commented at 4:14 AM on December 13, 2018: none

utACK, the code looks good.
lontivero force-pushed on Dec 13, 2018
lontivero renamed this:
~~rpc: Add test_accept_unsigned flag to testmempoolaccept~~
rpc: Allow testmempoolaccept to test unsigned transactions
on Dec 13, 2018
gmaxwell commented at 6:29 AM on December 13, 2018: contributor

Hm but many important standardness rules and consensus rules are part of script validation. As a result this will not achieve the stated goal of telling you if the transaction would be accepted but for being signed.
nopara73 commented at 3:48 PM on December 13, 2018: none

It would partly achieve it, the rest could be handled from code. For example unspentness, descendant and ascendant count limits would be checked properly. Descendant and ascendant size limits can be, too by smartly constructing the "fake transaction" described here: #14859#issue-386659270

I agree however that it can be considered misleading if too many standardness things depend on proper signatures, which I am not able to judge.

An alternative could be to give back multiple reject reasons instead of just the first reject reason encountered. This way the user could decide that if it wants to dismiss a reject reason or not. Of course, keeping in mind to not break other software, this could only be implemented by adding another json return tag: reject-reasons which would be confusing next to the existing reject-reason tag.
sdaftuar commented at 5:57 PM on December 13, 2018: member

I think the way this is implemented would be confusing to future maintainers of this code -- for example, if more policy checks were to be added in the future that take place inside script validation, it would be hard for us to determine whether it breaks the use case that this code change is trying to make.
gmaxwell commented at 6:10 PM on December 13, 2018: contributor

Dummying out checksigs (and maybe equalverify) might be a better way to go. With the exception of some fee limit stuff, you should generally expect almost all standardness checks to be signature validation flags.
lontivero commented at 8:39 PM on December 16, 2018: contributor

@gmaxwell, I am not sure to understand what you mean by dummying out checksigs and equalverify ops. Do you mean we could create a new DummyTransactionSignatureChecker class or something like that?
DrahtBot added the label Needs rebase on Dec 17, 2018
Add test_accept_unsigned flag to testmempoolaccept RPC method 22d9e65200
Replace test_accept boolean parameters by enum fed8ef3cbe
lontivero force-pushed on Dec 18, 2018
DrahtBot removed the label Needs rebase on Dec 18, 2018
lontivero closed this on Jan 16, 2019
MarcoFalke locked this on Dec 16, 2021

Contributors

Labels

Linked (view graph)

#14859 [Feature Request] testmempoolaccept without signature

rpc: Allow testmempoolaccept to test unsigned transactions #14939

Conflicts