1ef28b4f7c Make AnalyzePSBT next role calculation simple, correct (Gregory Sanders)
Pull request description:
Sniped test and alternative to https://github.com/bitcoin/bitcoin/pull/18220
Sjors documenting the issue:
```
A PSBT signed by ColdCard was analyzed as follows (see #17509 (comment))
{
"inputs": [
{
"has_utxo": true,
"is_final": false,
"next": "finalizer"
}
],
"estimated_vsize": 141,
"estimated_feerate": 1e-05,
"fee": 1.41e-06,
"next": "signer"
}
I changed AnalyzePSBT so that it returns "next": "finalizer" instead.
```
It makes it much clearer that the role has been decided before hitting the `calc_fee` block, and groups all state-deciding in one spot instead of 2.
Note that this assumes that PSBT roles are a complete ordering, which for now and in the future seems to be a correct assumption.
ACKs for top commit:
Sjors:
ACK 1ef28b4f7c, much nicer. Don't forget to document the bug fix.
achow101:
ACK 1ef28b4f7c
Empact:
ACK 1ef28b4f7c
Tree-SHA512: 22ba4234985c6f9c1445b14565c71268cfaa121c4ef000ee3d5117212b09442dee8d46d9701bceddaf355263fe25dfe40def2ef614d4f2fe66c9ce876cb49934
8bca30ea17 [rpc] expose ability to mock scheduler via the rpc (Amiti Uttarwar)
7c8b6e5b52 [lib] add scheduler to node context (Amiti Uttarwar)
930d837542 [test] add chainparams property to indicate chain allows time mocking (Amiti Uttarwar)
1cd43e83c6 [test] unit test for new MockForward scheduler method (Amiti Uttarwar)
a6f63598ad [util] allow scheduler to be mocked (Amiti Uttarwar)
Pull request description:
This PR is to support functional tests by allowing the scheduler to be mocked via the RPC.
It adds a `MockForward` method to the scheduler class that iterates through the task queue and reschedules them to be `delta_seconds` sooner.
This is currently used to support functional testing of the "unbroadcast" set tracking in #18038. If this patch is accepted, it would also be useful to simplify the code in #16698.
ACKs for top commit:
MarcoFalke:
ACK 8bca30ea17, only change is some style fixups 🕓
Tree-SHA512: 2a97fe8ade2b7fd1fb5cdfa1dcafb3227a377d7a847e3845a228bc119eb77824b4aefa43d922a06d583939b22725e223f308cf092961048079d36f6b1d9a639b
deaa6dd144 psbt: check output index is within bounds before accessing (Andrew Chow)
f1ef7f0aa4 Don't calculate tx fees for PSBTs with invalid money values (Andrew Chow)
Pull request description:
Fixes#17149
Two classes of issues were found by the psbt fuzzer: values out of range and causing overflows, and prevout indexes being out of range. This PR fixes both.
When accessing a specific output using the index given in the tx, check that it is actually a possible output before trying to access the output.
When summing and checking amounts for `decodepsbt` and `analyzepsbt`, make sure that the values are actually valid money values.. Otherwise, stop summing and don't show the fee. For `analyzepsbt`, return that the next role is the Creator since the Creator needs to remake the transaction to be valid.
ACKs for top commit:
practicalswift:
ACK deaa6dd144 -- only change since last ACK was the addition of tests
gwillen:
tested ACK deaa6dd, would also like to see this merged!
Tree-SHA512: 06c36720bbb5a7ab1c29f7d15878bf9f0d3e5760c06bff479d412e1bf07bb3e0e9ab6cca820a4bfedaab71bfd7af813807e87cbcdf0af25cc3f66a53a06dbcfd
Identified via -Wdocumentation, e.g.:
./rpc/rawtransaction_util.h:31:13: error: parameter 'prevTxs' not found in the function declaration [-Werror,-Wdocumentation]
* @param prevTxs Array of previous txns outputs that tx depends on but may not yet be in the block chain
^~~~~~~
./rpc/rawtransaction_util.h:31:13: note: did you mean 'prevTxsUnival'?
* @param prevTxs Array of previous txns outputs that tx depends on but may not yet be in the block chain
^~~~~~~
prevTxsUnival
netbase.cpp:766:11: error: parameter 'outProxyConnectionFailed[out]' not found in the function declaration [-Werror,-Wdocumentation]
* @param outProxyConnectionFailed[out] Whether or not the connection to the
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
netbase.cpp:766:11: note: did you mean 'outProxyConnectionFailed'?
* @param outProxyConnectionFailed[out] Whether or not the connection to the
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
outProxyConnectionFailed
fa8e650b52 rest: Use mempool from node context instead of global (MarcoFalke)
fa660d65d7 node: Use mempool from node context instead of global (MarcoFalke)
facbaf092f rpc: Use mempool from node context instead of global (MarcoFalke)
Pull request description:
Currently they are identical, but in the future we might want to turn
the mempool into a unique_ptr. Replacing the global with the mempool
pointer from the node context simplifies this step.
ACKs for top commit:
jnewbery:
Code review ACK fa8e650b5
ryanofsky:
Code review ACK fa8e650b52, Only the discussed REST server changes since the last review.
Tree-SHA512: 0836f3f39cf90306455962918446e5f8612e88c32072b92afc30929aea1f17430bbda0e2b3668d36c9d6b97d63a93cf4903185194571108642b7bf5a39b89125
In decodepsbt if an invalid amount is seen, don't calculate the fee
but still show the invalid value in the decode.
In analyze psbt, if an invalid amount is seen, set the next step to
be the creator as the creator needs to remake the transaction so that
it is valid.
773d4572a4 Mark PSBTs spending unspendable outputs as invalid in analysis (Andrew Chow)
638e40cb60 Have a PSBTAnalysis state that indicates invalid PSBT (Andrew Chow)
Pull request description:
When analyzing an unspendable PSBT, report that it is unspendable and exit analysis early.
ACKs for top commit:
Sjors:
ACK 773d457
instagibbs:
After some thought ACK 773d4572a4
Tree-SHA512: 99b0cb2fa1ea37593fc65a20effe881639d69ddeeecf5197bc87bc7f2220cbeb40f1d429d517e4d27f2e9fb563a00cd845d2b4b1ce05246a75a6cb56fb9b0ba5
Invalid PSBTs need to be re-created, so the next role is the
Creator (new PSBTRole). Additionally, we need to know what went
wrong so an error field was added to PSBTAnalysis.
A PSBTAnalysis indicating invalid will have empty everything,
next will be set to PSBTRole::CREATOR, and an error message.
Currently it is an alias to the global ::mempool and should be used as
follows.
* Node code (validation and transaction relay) can use either ::mempool
or node.mempool, whichever seems a better fit.
* RPC code should use the added convenience getter EnsureMempool, which
makes sure the mempool exists before use. This prepares the RPC code
to a future where the mempool might be disabled at runtime or compile
time.
* Test code should use m_node.mempool directly, as the mempool is always
initialized for tests.
92b2f5306b test: add dumptxoutset RPC test (James O'Beirne)
c1ccbc3dde devtools: add utxo_snapshot.sh (James O'Beirne)
57cf74c991 rpc: add dumptxoutset (James O'Beirne)
92fafb3a7d coinstats: add coins_count (James O'Beirne)
707fde7b9b add unused SnapshotMetadata class (James O'Beirne)
Pull request description:
This is part of the [assumeutxo project](https://github.com/bitcoin/bitcoin/projects/11):
Parent PR: #15606
Issue: #15605
Specification: https://github.com/jamesob/assumeutxo-docs/tree/master/proposal
---
This changeset defines the serialization format for UTXO snapshots and adds an RPC command for creating them, `dumptxoutset`. It also adds a convenience script for generating and verifying snapshots at a certain height, since that requires doing a hacky rewind of the chain via `invalidateblock`.
All of this is unused at the moment.
ACKs for top commit:
laanwj:
ACK 92b2f5306b
Tree-SHA512: 200dff87767f157d627e99506ec543465d9329860a6cd49363081619c437163a640a46d008faa92b1f44fd403bfc7a7c9e851c658b5a4849efa9a34ca976bf31
3004d5a12d [validation] Remove fMissingInputs from AcceptToMemoryPool() (John Newbery)
c428622a5b [validation] Remove unused first_invalid parameter from ProcessNewBlockHeaders() (John Newbery)
7204c6434b [validation] Remove useless ret parameter from Invalid() (John Newbery)
1a37de4b31 [validation] Remove error() calls from Invalid() calls (John Newbery)
067981e492 [validation] Tidy Up ValidationResult class (John Newbery)
a27a2957ed [validation] Add CValidationState subclasses (John Newbery)
Pull request description:
Carries out some remaining tidy-ups remaining after PR 15141:
- split ValidationState into TxValidationState and BlockValidationState (commit from ajtowns)
- various minor code style tidy-ups to the ValidationState class
- remove the useless `ret` parameter from `ValidationState::Invalid()`
- remove the now unused `first_invalid` parameter from `ProcessNewBlockHeaders()`
- remove the `fMissingInputs` parameter from `AcceptToMemoryPool()`, and deal with missing inputs the same way as other errors by using the `TxValidationState` object.
Tip for reviewers (thanks ryanofsky!): The first commit ("[validation] Add CValidationState subclasses" ) is huge and can be easier to start reviewing if you revert the rote, mechanical changes:
Substitute the commit hash of commit "[validation] Add CValidationState subclasses" for <CommitHash> in the commands below.
```sh
git checkout <CommitHash>
git grep -l ValidationState | xargs sed -i 's/BlockValidationState\|TxValidationState/CValidationState/g'
git grep -l ValidationResult | xargs sed -i 's/BlockValidationResult\|TxValidationResult/ValidationInvalidReason/g'
git grep -l MaybePunish | xargs sed -i 's/MaybePunishNode\(ForBlock\|ForTx\)/MaybePunishNode/g'
git diff HEAD^
```
After that it's possible to easily see the mechanical changes with:
```sh
git log -p -n1 -U0 --word-diff-regex=. <CommitHash>
```
ACKs for top commit:
laanwj:
ACK 3004d5a12d
amitiuttarwar:
code review ACK 3004d5a12d. Also built & ran tests locally.
fjahr:
Code review ACK 3004d5a12d . Only nit style change and pure virtual destructor added since my last review.
ryanofsky:
Code review ACK 3004d5a12d. Just whitespace change and pure virtual destructor added since last review.
Tree-SHA512: 511de1fb380a18bec1944ea82b513b6192df632ee08bb16344a2df3c40811a88f3872f04df24bc93a41643c96c48f376a04551840fd804a961490d6c702c3d36
These procedures will later be used in the ChainstateManager to compute
statistics (particularly a content hash) for UTXO sets coming in from
snapshots.
582d2cd747 Cover UTXO set access with lock annotations (James O'Beirne)
5693530685 refactor: have CCoins* data managed under CChainState (James O'Beirne)
fae6ab6aed refactor: pcoinsTip -> CChainState::CoinsTip() (James O'Beirne)
Pull request description:
This is part of the [assumeutxo project](https://github.com/bitcoin/bitcoin/projects/11):
Parent PR: #15606
Issue: #15605
Specification: https://github.com/jamesob/assumeutxo-docs/tree/2019-04-proposal/proposal
---
This change encapsulates UTXO set data within CChainState instances, removing global data `pcoinsTip` and `pcoinsviewdb`. This is necessary if we want to maintain multiple chainstates with their own rendering of the UTXO set.
We introduce a class CoinsViews which consolidates the construction of a CCoins* hierarchy.
This commit could be broken into smaller pieces, but it would require more ephemeral diffs to, e.g., temporarily change CCoinsViewDB's constructor invocations.
ACKs for top commit:
Sjors:
reACK 582d2cd747
MarcoFalke:
ACK 582d2cd747
Tree-SHA512: ec9d904fe5dca8cd2dc4b7916daa5d8bab30856dd4645987300f905e0a19f9919fce4f9d1ff03eda982943ca73e6e9a746be6cf53b46510de36e8c81a1eafba1
RPC server starts in warmup mode, it can't
process yet calls, then follows connection manager
initialization and finally RPC server get out of
warmup mode. RPC calls shouldn't be able to get
P2P disabled errors because once we initialize
g_connman it's not unset until shutdown, after
RPC server has been stopped.
Access through a broadcastTransaction method.
Add a wait_callback flag to turn off race protection when wallet
already track its tx being in mempool
Standardise highfee, absurdfee variable name to max_tx_fee
We drop the P2P check in BroadcastTransaction as g_connman is only
called by RPCs and the wallet scheduler, both of which are initialized
after g_connman is assigned and stopped before g_connman is reset.
psbt.cpp definitions except for AnalyzePSBT are used by the wallet and need to
be linked into the wallet binary. AnalyzePSBT is an exception in that it is not
used by the wallet, and depends on node classes like CCoinsViewCache, and on
node global variables like nBytesPerSigOp.
So AnalyzePSBT is more at home in libbitcoin_server than libbitcoin_common, and
in any case needs to be defined in a separate object file than other PSBT
utilities, to avoid dragging link dependencies on node functions and global
variables into the wallet.
Adds the following util units and adds them to libbitcoin_util:
- `util/url.cpp` takes `urlDecode` from `httpserver.cpp`
- `util/error.cpp` takes `TransactionErrorString` from
`node/transaction.cpp` and `AmountHighWarn` and `AmountErrMsg` from
`ui_interface.cpp`
- `util/fees.cpp` takes `StringForFeeReason` and `FeeModeFromString` from `policy/fees.cpp`
- `util/rbf.cpp` takes `SignalsOptInRBF` from `policy/rbf.cpp`
- 'util/validation.cpp` takes `FormatStateMessage` and `strMessageMagic` from 'validation.cpp`
Refactor the new CombinePSBT, FinalizePSBT, and FinalizeAndExtractPSBT
general-purpose functions out of the combinepsbt and finalizepsbt RPCs,
for use in the GUI code.
After refactoring general-purpose PSBT and transaction code out of RPC code,
for use in the GUI, it's no longer appropriate to throw exceptions. Instead we
now return bools for success, and take an output parameter for an error object.
We still use JSONRPCError() for the error objects, since only RPC callers
actually care about the error codes.
Factor out a new BroadcastTransaction function, performing the core work of the
sendrawtransaction rpc, so that it can be used from the GUI code. Move it from
src/rpc/ to src/node/.
Ben Woosley