Changes API for AcceptToMemoryPool to no longer check maximum fee; we remove
this check from interfaces::CChain::testPeginClaimAcceptance. This is fine,
the check never should have been there (this method's job is to notice double
claim attempts and provide a more helpful message to the user; other mempool
failures should be caught later in the `claimrawpegin` logic when we call
`pwallet->commitTransaction` which actually subits to the mempool). It was
only checking fees because the API made it easier to do so than to not.
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.
This resulted in double-initializing the asset directory (and from multiple
places), so I added a `ClearGlobalAssetDir` method and called it from the
qt test setup.
Added a new "signed but not finalized" vector to the functional tests, which took
some time to figure out how to generate. Evenetually I took the output of walletsignpsbt
from the nonconf->nonconf test and manually un-finalized that by pulling the
scriptwitness apart into a pubkey->signature mapping.
This fixes a segfault I introduced in 981df590811 (#17371) a couple hundred PRs
ago, where we might out-of-bounds access the vout arrray of a non_witness_utxo
transaction in a PSBT.
This logic should probably be refactored after the rebase -- we sift through
the non_witness_utxos to find a scriptpubkey, which we use to get a wallet
provider, so we can pass this to SignPSBTInput, which then repeats the same
logic (for its own reasons, and without the segfault bug)....and we aren't
even calling SignPSBTInput for signing reasons! It has something to do with
blinding.
Check absurd fee in BroadcastTransaction and RPC,
return TransactionError::MAX_FEE_EXCEEDED instead
of TxValidationResult::TX_NOT_STANDARD because this
is client preference, not a node-wide policy.
a8a64acaf3 [BroadcastTransaction] Remove unsafe move operator (Amiti Uttarwar)
125c038126 [p2p] Remove dead code (Amiti Uttarwar)
fc66d0a65c [p2p] Check for nullptr before dereferencing pointer (Adam Jonas)
cb79b9dbf4 [mempool] Revert unbroadcast set to tracking just txid (Amiti Uttarwar)
Pull request description:
Addresses some outstanding review comments from #18044
- reverts unbroadcast txids to a set instead of a map (simpler, communicates intent better, takes less space, no efficiency advantages of map)
- adds safety around two touchpoints (check for nullptr before dereferencing pointer, remove an inaccurate std::move operator)
- removes some dead code
Links to comments on wtxid PR: [1](https://github.com/bitcoin/bitcoin/pull/18044#discussion_r460495254) [2](https://github.com/bitcoin/bitcoin/pull/18044#discussion_r460496023) [3](https://github.com/bitcoin/bitcoin/pull/18044#discussion_r463532611)
thanks to jnewbery & adamjonas for flagging these ! !
ACKs for top commit:
sdaftuar:
utACK a8a64acaf3
naumenkogs:
utACK a8a64acaf3
jnewbery:
utACK a8a64acaf3
Tree-SHA512: 7be669cb30cc17fb9e06b50e636ef7887c6a27354697987e4e4d38dba4b8f50e175647587430cd9bc3295bec01ce8b1e6639a50a4249d8fff9b1ca1b9ead3277
