7aab8d1024 [style] Code style fixups in GetWarnings() (John Newbery)
492c6dc1e7 util: change GetWarnings parameter to bool (John Newbery)
869b6314fd [qt] remove unused parameter from getWarnings() (John Newbery)
Pull request description:
`GetWarnings()` changes the format of the output warning string based on a passed-in string argument that can be set to "gui" or "statusbar".
Change the argument to a bool:
- there are only two types of behaviour, so a bool is a more natural argument type
- changing the name to `verbose` does not set any expectations for the how the calling code will use the returned string (currently, `statusbar` is used for RPC warnings, not a status bar)
- removes some error-handling code for when the passed-in string is not one of the two strings expected.
ACKs for top commit:
laanwj:
code review ACK 7aab8d1024
practicalswift:
ACK 7aab8d1024 -- diff looks correct :)
MarcoFalke:
ACK 7aab8d1024 otherwise.
promag:
Code review ACK 7aab8d1024.
Tree-SHA512: 75882c6e3e44aa9586411b803149b36ba487f4eb9cac3f5c8f07cd9f586870bba4488a51e674cf8147f05718534f482836e6a4e3f66e0d4ef6821900c7dfd04e
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
GetWarnings() changes the format of the output warning string based on a
passed-in string argument that can be set to "gui" or "statusbar".
Change the argument to a bool:
- there are only two types of behaviour, so a bool is a more natural
argument type
- changing the name to 'verbose' does not set any expectations for the
how the calling code will use the returned string (currently,
'statusbar' is used for RPC warnings, not a status bar)
- removes some error-handling code for when the passed-in string is not
one of the two strings expected.
7d263571be rpc: require second argument only for scantxoutset start action (Andrew Chow)
Pull request description:
It was reported on [IRC](http://www.erisian.com.au/bitcoin-core-dev/log-2019-12-11.html#l-377) that `scantxoutset`'s API was broken in 0.19.0:
```
<belcher> i think scantxoutset may have been broken in bitcoin core 0.19 ? regardless of what parameters i run it with (e.g. "scantxoutset abort", "scantxoutset status") it just returns the help doc, according to the release notes the only change was https://github.com/bitcoin/bitcoin/pull/16285/files but i dont see anything that wouldve broken it, it works fine in 0.18
<belcher> im on regtest, in case its important
<harding> I can confirm `scantxoutset abort` returns the help doc on latest master. Waiting for 0.18.1 to start now to attempt to reproduce there.
<harding> It looks like it's expecting a second parameter (even though that doesn't make sense with "abort").
<jonatack> Same for me as well
<harding> Can also confirm that `scantxoutset abort` returns the expected result on 0.18.1.
```
As noted in the conversation, previously, the second argument of `scanobjects` is only required for the `start` action. `Stop` and `abort` actions did not and could work without them.
It appears that this was broken by #16240 which enforced the size of the arguments to match the listed required arguments.
To fix this issue, this PR makes the `scanobjects` argument an optional argument. Then only in the `start` action do we check whether the `scanobjects` argument is there and throw an informative error about that. Also a test is added for this case.
ACKs for top commit:
laanwj:
ACK 7d263571be
promag:
ACK 7d263571be.
Tree-SHA512: 828bdfe47f4fffa5d00a2cf88db6cea4a2714d9c49276841ca5cbdd1603b87bb6862147b86edcf36d7b40314ddb80b1a07fd399faf288572c55cc788c5cf9526
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.
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
c98bd13e67 replace asserts in RPC code with CHECK_NONFATAL and add linter (Adam Jonas)
Pull request description:
- Replace instances of assert in /rpc files and rpcwallet with CHECK_NONFATAL(condition)
- Add a linter to prevent future usage of assert being used in RPC code
ref https://github.com/bitcoin/bitcoin/pull/17192
ACKs for top commit:
practicalswift:
ACK c98bd13e67 -- diff looks correct
Tree-SHA512: a16036b6bbcca73a5334665f66e17e1756377d582317568291da1d727fc9cf8c84bac9d9bd099534e1be315345336e5f7b66b93793135155f320dc5862a2d875
3ed8e3d079 doc: Remove explicit network name references (Fabian Jahr)
d6e493f0c2 wallet: Remove left-over BIP70 comment (Fabian Jahr)
Pull request description:
A small follow-up to #17165 which removed BIP70 support.
1. Removes one leftover mention of BIP70 in a comment.
2. Removes BIP70 reference in comments on network/chain name strings. These can be removed as they are not really helpful and also incorrect: BIP70 only defines "main" and "test" but not "regtest". If/When signet gets merged we will add another name to the list that is not defined in BIP70. Mostly there is also an exhaustive list of the options included in the comment anyway.
If we would like to keep an identifier for this naming scheme, I would suggest switching to something more generic, like 'short chain name'. Happy to implement that if that is preferred. Alternatively, we could add a reference to `CBaseChainParams`. That would also mean we don't have to change these lines again for signet.
ACKs for top commit:
MarcoFalke:
ACK 3ed8e3d079
Tree-SHA512: 9a7c0b9cacbb67bd31a089ffdc6f1ebc7f336493e2c8266eb697da34dce2b505a431d5639a3e4fc34f9287361343e861b55dc2662e0a1d2095cc1046db77d6ee
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
Wallet code should use interfaces::Chain and not directly access to node state.
Add a g_rpc_chain replacement global for wallet code to use, and move
g_rpc_node definition to a libbitcoin_server source file so there are link
errors if wallet code tries to access it.
faec689bed txmempool: Make entry time type-safe (std::chrono) (MarcoFalke)
faaa1f01da util: Add count_seconds time helper (MarcoFalke)
1111170f2f test: mempool entry time is persisted (MarcoFalke)
Pull request description:
This changes the type of the entry time of txs into the mempool from `int64_t` to `std::chrono::seconds`.
The benefits:
* Documents the type for developers
* Type violations result in compile errors
* After compilation, the two are equivalent (at no run time cost)
ACKs for top commit:
ajtowns:
utACK faec689bed
laanwj:
ACK faec689bed
Tree-SHA512: d958e058755d1a1d54cef536a8b30a11cc502b7df0d6ecf84a0ab1d38bc8105a67668a99cd5087a444f6de2421238111c5fca133cdf8e2e2273cb12cb6957845
The response in the RPC result `starttime` is camel cased while the rest of the response seems to be lower cased.
If this was intentional please ignore this PR.
Note: case might break existing callers
Reflect the change in the test data
Change to snake case
bdd6a4fd5d qa: Check scantxoutset result against gettxoutsetinfo (João Barbosa)
fc0c410d6e rpc: Improve scantxoutset response and help message (João Barbosa)
Pull request description:
The new response keys `height` and `bestblock` allow the client to know at what point the scan took place.
The help message now has all the response keys (`result` and `txouts` were missing) and it's improved a bit. Note that `searched_items` key is renamed to `txouts`, considering `scantxoutset` is marked experimental.
ACKs for top commit:
laanwj:
ACK bdd6a4fd5d
Tree-SHA512: 6bb7c3464b19857b756b8bc491ab7c58b0d948aad8c005b26ed27c55a1278f5639217e11a315bb505b4f44ebe86f413068c1e539c8a5f7a4007735586cc6443c
d48c1e837a Add window final block height to getchaintxstats (Jonathan "Duke" Leto)
Pull request description:
This patch is motivated by the desire to make the output of `getchaintxstats` more useful and optimized for applications to consume and render the data.
Firstly, this data is already available to the RPC, no additional work is done. Currently additional RPC calls will be needed to look up the height of the final block in the window or the block height that began the window.
By adding the block height of the final block in the window, the JSON is "self-contained" and applications can calculate the exact block height range of the window with no additional RPC requests.
For example, a web application which wants to render historical information for `getchaintxstats` RPC on various window sizes might call the RPC with various window lengths, once per day, and store the JSON results somewhere. Because the final block height of each dataset is included, it's no extra work to determine the exact block window range of each JSON response.
ACKs for top commit:
promag:
ACK d48c1e837a.
Tree-SHA512: fd4952c125f81a4ad18f7c78498c6b3e265b93cb574832166ac25596321ce84957f971f3f78f37d7e42638dc65f2a5d4d760f289873c9c2f2a82eb00a0f87c3f
These procedures will later be used in the ChainstateManager to compute
statistics (particularly a content hash) for UTXO sets coming in from
snapshots.
e78aaf41f4 [docs] Add release notes for burying bip 9 soft fork deployments (John Newbery)
8319e738f9 [tests] Add coverage for the content of getblockchaininfo.softforks (James O'Beirne)
0328dcdcfc [Consensus] Bury segwit deployment (John Newbery)
1c93b9b31c [Consensus] Bury CSV deployment height (John Newbery)
3862e473f0 [rpc] Tidy up reporting of buried and ongoing softforks (John Newbery)
Pull request description:
This hardcodes CSV and segwit activation heights, similar to the BIP 90 buried deployments for BIPs 34, 65 and 66.
CSV and segwit have been active for over 18 months. Hardcoding the activation height is a code simplification, makes it easier to understand segwit activation status, and reduces technical debt.
This was originally attempted by jl2012 in #11398 and again by me in #12360.
ACKs for top commit:
ajtowns:
ACK e78aaf41f4 ; checked diff to previous acked commit, checked tests still work
ariard:
ACK e78aaf4, check diff, run the tests again and successfully activated csv/segwit heights on mainnet as expected.
MarcoFalke:
ACK e78aaf41f4 (still didn't check if the mainnet block heights are correct, but the code looks good now)
Tree-SHA512: 7e951829106e21a81725f7d3e236eddbb59349189740907bb47e33f5dbf95c43753ac1231f47ae7bee85c8c81b2146afcdfdc11deb1503947f23093a9c399912
i.e. any CoinsViews members. Adds a lock acquisition to `gettxoutsetinfo` RPC
to comply with added annotations.
Co-authored-by: Russell Yanofsky <russ@yanofsky.org>
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. Construction of its various pieces (db, coinscatcher,
in-memory cache) is split up so that we avoid flushing bad state to disk if
startup is interrupted.
We also introduce `CChainState::CanFlushToDisk()` which tells us when it is
safe to flush the chainstate based on this partial construction.
This commit could be broken into smaller pieces, but it would require more
ephemeral diffs to, e.g., temporarily change CCoinsViewDB's constructor
invocations.
Other changes:
- A parameter has been added to the CCoinsViewDB constructor that allows the
name of the corresponding leveldb directory to be specified.
Thanks to Russell Yanofsky and Marco Falke for helpful feedback.
This combines reporting of buried (formally ISM) softfork deployments
and BIP9 versionbits softfork deployments into one JSON object in the
getblockchaininfo return object.
682a1d0f20 refactoring: remove mapBlockIndex global (James O'Beirne)
55d525ab90 refactoring: make pindexBestInvalid internal to validation.cpp (James O'Beirne)
4ed55dfcd7 refactoring: add block_index_candidates arg to LoadBlockIndex (James O'Beirne)
613c46fe9e refactoring: move block metadata structures into BlockManager (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
---
Under an assumeutxo model, we have multiple CChainState instances in use at once in order to support background validation. Currently, each CChainState instance has its own mapBlockIndex, a collection of linked block headers, in addition to a few other data structures that are related to maintenance of the block tree but not necessarily to any given chainstate.
In order to avoid duplicating this data across chainstates, this change moves chainstate-agnostic block metadata (and related behavior) into a class, `BlockManager`. Chainstates are parameterized with a reference to a blockmanager instance and in practice they share the same instance.
Most of this change is conceptually move-only, though the diff is somewhat muddled. The first commit can be reviewed slightly more easily with `--color-moved=dimmed_zebra`. Admittedly, that commit is pretty unwieldy; I tried to split it up after the fact with `git add --patch`, but that was difficult because of git's inability to split hunks past a certain point. Some of the moves also ended up being obscured when done over separate commits.
ACKs for top commit:
MarcoFalke:
ACK 682a1d0f20
ryanofsky:
utACK 682a1d0f20, only changes since last review were rebase and fixing conflict on a moved line
ariard:
utACK 682a1d0. Most of the changes are move-only, with main problem being to avoid creating circular dependencies between `BlockManager` and `CChainState`. Tested, comments are mostly nits, feel free to ignore them
Tree-SHA512: 738d8d06539ba53acf4bd2d48ae000473e645bbc4e63d798d55d247a4d5a4f781b73538ed590f6407be9ab402ea9d395570ea20bff0a4b9ce747bcc1600c5108
b6fb617aaa rpc: switch to using RPCHelpMan.Check() (Karl-Johan Alm)
c7a9fc234f Make the RPCHelpMan aware of JSONRPCRequest and add Check() helper (Karl-Johan Alm)
5c5e32bbe3 rpc: migrate JSONRPCRequest functionality into request.cpp (Karl-Johan Alm)
0ab8ba1ac6 rpc: fix RPC help requirements for getblocktemplate (Karl-Johan Alm)
Pull request description:
Every single RPC call has a helper-section at the start, which throws a help string if the user asks for help or if the user provided too few/many arguments.
```C++
const RPCHelpMan help{...};
if (request.fHelp || !help.IsValidNumArgs(request.params.size())) {
throw std::runtime_error(help.ToString());
}
```
or (older version)
```C++
if (request.fHelp || request.params.size() < min || request.params.size() > max)
throw std::runtime_error(
RPCHelpMan{...}.ToString()
);
```
It seems like an obvious improvement, and less copy-pasting, to make `RPCHelpMan` aware of `JSONRPCRequest`, and to let it handle the checks instead. Both of the above become
```C++
RPCHelpMan{...}.Check(request);
```
which means we save roughly 3 lines per RPC command, and the `RPCHelpMan` instance is never referenced afterwards, so the approach is a tiny fraction cleaner.
This is a complete update, sans a few special case locations that had special rules. 623 lines turn into 284 (which includes the addition to `RPCHelpMan`).
ACKs for top commit:
laanwj:
code rview and lightly tested ACK b6fb617aaa
MarcoFalke:
ACK b6fb617aaa, looked at the diff, verified move-only where applicable
Tree-SHA512: eb73f47f812512905b852e313281d1c8df803db40a6188aa39d5a7586631664db6764491152a8a96769946c796dc56d38c6e3a66ddd06ba3fb9d20050e6274e1
26fe9b9909 Add support for descriptors to utxoupdatepsbt (Pieter Wuille)
3135c1a2d2 Abstract out UpdatePSBTOutput from FillPSBT (Pieter Wuille)
fb90ec3c33 Abstract out EvalDescriptorStringOrObject from scantxoutset (Pieter Wuille)
eaf4f88734 Abstract out IsSegWitOutput from utxoupdatepsbt (Pieter Wuille)
Pull request description:
This adds a descriptors argument to the `utxoupdatepsbt` RPC. This means:
* Input and output scripts and keys will be filled in when known.
* P2SH-witness inputs will be filled in from the UTXO set when a descriptor is provided that shows they're spending segwit outputs.
This also moves some (newly) shared code to separate functions: `UpdatePSBTOutput` (an analogue to `SignPSBTInput`), `IsSegWitOutput`, and `EvalDescriptorStringOrObject` (implementing the string or object notation parsing used in `scantxoutset`).
ACKs for top commit:
jnewbery:
utACK 26fe9b9909
laanwj:
utACK 26fe9b9909 (will hold merging until response to promag's comments)
promag:
ACK 26fe9b9, checked refactors and tests look comprehensive. Still missing a release note but can be added later.
Tree-SHA512: 1d833b7351b59d6c5ded6da399ff371a8a2a6ad04c0a8f90e6e46105dc737fa6f2740b1e5340280d59e01f42896c40b720c042f44417e38dfbee6477b894b245
67f4e9c522 Include core_io.h from core_read.cpp (practicalswift)
eca9767673 Make reasoning about dependencies easier by not including unused dependencies (practicalswift)
Pull request description:
Make reasoning about dependencies easier by not including unused dependencies.
Please note that the removed headers are _not_ "transitively included" by other still included headers. Thus the removals are real.
As an added bonus this change means less work for the preprocessor/compiler. At least 51 393 lines of code no longer needs to be processed:
```
$ git diff -u HEAD~1 | grep -E '^\-#include ' | cut -f2 -d"<" | cut -f1 -d">" | \
sed 's%^%src/%g' | xargs cat | wc -l
51393
```
Note that 51 393 is the lower bound: the real number is likely much higher when taking into account transitively included headers :-)
ACKs for commit 67f4e9:
Tree-SHA512: 0c8868aac59813f099ce53d5307eed7962dd6f2ff3546768ef9e5c4508b87f8210f1a22c7e826c3c06bebbf28bdbfcf1628ed354c2d0fdb9a31a42cefb8fdf13
403e677c9 refactoring: IsInitialBlockDownload -> CChainState (James O'Beirne)
3ccbc376d refactoring: FlushStateToDisk -> CChainState (James O'Beirne)
4d6688603 refactoring: introduce ChainstateActive() (James O'Beirne)
d7c97edee move-only: make the CChainState interface public (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 changeset starts moving functionality intimately related to CChainState into methods. Parameterizing these functions by a particular CChainState is necessary for the use of multiple chainstates simultaneously (e.g. for asynchronous background validation).
In this change, we
- make the CChainState interface public - since other units will start to invoke its methods directly,
- introduce `::ChainstateActive()`, the CChainState equivalent for `::ChainActive()`,
- and move `IsInitialBlockDownload()` and `FlushStateToDisk()` into methods on CChainState.
Independent of assumeutxo, these changes better encapsulate chainstate behavior and allow easier use from a testing context.
There are more methods that we'll move in the future, but they require other substantial changes (i.e. moving ownership of the `CCoinsView*` hierarchy into CChainState) so we'll save them for future PRs.
---
The first move-only commit is most easily reviewed with `git diff ... --color-moved=dimmed_zebra`.
ACKs for commit 403e67:
Empact:
utACK 403e677c9e no need to address my nits herein
Sjors:
utACK 403e677
ryanofsky:
utACK 403e677c9e. Only change since previous review is removing global state comment as suggested.
MarcoFalke:
utACK 403e677c9e, though the diff still seems a bit bloated with some unnecessary changes in the second commit.
promag:
utACK 403e677 and rebased with current [master](c7cfd20a7).
Tree-SHA512: 6fcf260bb2dc201361170c0b4547405366f5f331fcc3a2bac29b24442814b7b244ca1b58aac5af716885f9a130c343b544590dff780da0bf835c7c5b3ccb2257
Also renames global methods for clarity:
- ::FlushStateToDisk() -> CChainState::ForceFlushStateToDisk()
- This performs an unconditional flush.
- ::PruneAndFlush() -> CChainState::PruneAndFlush()
