Spent a while figuring out where this new function should live now that
the PSBT stuff has been pulled into src/psbt.cpp and the wallet, and isn't
mixed up with the RPC logic.
In the end I modified the the function to return a normal error rather
than throwing an RPC exception and put it in src/psbt.cpp with the other
functions that behave this way.
We have a couple RPC hacks in src/wallet/rpcwallet.cpp that were a bit annoying
to adapt, but nothing too bad.
Found a misspelling of `ignoreblindfail` in the `sendtoaddress` args, and a
discrepancy in the `createrawpegin`/`createpegin` args
Adds a new "send" RPC which I didn't really look at too closely to match
our other RPC modifications. In particular our backport of #17211 adds a
"solving_data" field to other transaction-creation RPCs, but not this
one. (But I checked the current status of #17211 and Andy hasn't updated
upstream either, just passes NullUniValue to FundTransaction from `send`.
So that's what I did here.)
Added a method testPeginClaimAcceptance to interfaces::CChain. It is not clear
to me that this is meaningfully different from the existing method broadcastTransaction
(with the `relay` flag set to false so it doesn't actually broadcast), except
that this method returns the TxValidationState, which we display to the user
in the RPC error. Worth revisiting this.
Another nonobvious change was replacing a block of mempool-searching logic
in wallet/rpcwallet.cpp with the single line `pwallet->chain().findCoins(coins)`.
This I stole from the current state of upstream #17211, our favorite in-progress
PR from which the original logic came.
Note that this is a breaking change in Core's RPC -- if you add pegins
to createfundedpsbt then it won't do coin selection unless you provide
the additional new add_inputs option.
Also re-`static`'d a method in src/script/standard.cpp which we had made globally
visible for some historical reason, but which I noticed diffing that file against
upstream.
For reasons I do not really grok, this PR changes the timing behavior of the
functional tests to reliably expose a deadlock in the claimpegin RPC that has
existed since the 0.17 rebase.
The mechanism is: in `claimpegin` in src/wallet/rpcwallet.cpp:5873, we call
`AcceptToMemoryPoolWorker`. This requires cs_main to be locked, which it is
not (contrast Core's `testmempoolaccept` RPC, which similarly calls
`AcceptToMemoryPoolWorker` from the RPC thread, and locks cs_main immediately
before).
We do *say* that it is locked, in the `LockAssertion` one the line above, but
this was added in ad3d496d78 during the 0.17
rebase (PR #620), apparently to shut up some linter on OSX, and as near as I
can tell it was never true.
Anyway, `AcceptToMemoryPoolWorker` calls through a couple layers which assume
cs_main is locked, to `AcceptSingleTransaction`, which locks m_pool.cs on line
src/validation.cpp:1101. It then calls `PreChecks`, which on line 784 calls
::ChainActive(), which _actually_ locks cs_main, though only briefly. This
line is the deadlock, because we locked m_pool.cs followed by cs_main...
...meanwhile, in src/net_processing.cpp, we lock cs_main at the top of the
`PeerLogicValidation::SendMessages` loop (src/net_processing.cpp:3628). In the
same loop, in the `feefilter` message processing, we call CTxMemPool::GetMinFee
(src/net_processing.cpp:4137), which locks m_pool.cs. Deadlock.
Anyway, that explains the change to locking behavior that I added to an
otherwise test-only PR.
This is the 43-commit descriptor wallet PR. It was remarkably easy to merge, given
its magnitude. With this commit Elements supports importing Bitcoin descriptors
and deriving (Bitcoin) addresses, though of course it does not support blinding
yet. That is a post-rebase project.
The material changes were:
1. Changing constants in the tests (super annoying but nothing surprising)
2. Adding a missing "skip if this coin is not ours" check in src/script/sign.cpp
which was causing us to erroneously remove existing witnesses from transactions.
This wasn't a problem before this commit since we would only ask specific
scriptpubkeymans to sign, and we'd never ask any to sign inputs we didn't
own. Andy simplified the logic here to always try every scriptpubkeyman,
which means they have to play a bit more nicely with each other.
Other than that, this was a big diff with many conflicts but literally all of
them were "we both added code" and the resolution was to take both sides.
