709998467e rpc: doc: Fix and extend getblockstats examples (Adam Soltys)
Pull request description:
This pull fixes the example curl command for `getblockstats` which doesn't work as is because it's missing a comma between the params and has single quotes around the second parameter.
It also adds an additional example of getting block stats by hash by using a known workaround (#15412) to get bitcoin-cli to treat the hash parameter as JSON instead of a string since there is ongoing deliberation about how or whether to fix the root issue (#15448).
ACKs for top commit:
theStack:
ACK 709998467e
Tree-SHA512: 84a5b7f449f06fff785bc0afbc1a7dfd55454bc76c52a8945e91556f87f3edfdc5a1780faab8fcfd6c415b734295b7c67d2e04ba7b6cfa91a77758af5dda53ae
This fixes the example curl command for `getblockstats` which is missing
a comma between the params and has single quotes around the second
parameter.
Besides fixing the existing example, this commit adds an additional
example of getting block stats by hash by using a known workaround with
bitcoin-cli to get it to treat the hash parameter as a JSON string by
wrapping it in both single and double quotes.
Co-Authored-By: Andrew Toth <andrewstoth@gmail.com>
Co-Authored-By: Sebastian Falbesoner <sebastian.falbesoner@gmail.com>
fa168d7542 rpc: Document all aliases for first arg of listtransactions (MarcoFalke)
fa5b1f067f rpc: Document all aliases for second arg of getblock (MarcoFalke)
fa86a4bbfc rpc: Rename first arg of generateblock RPC to "output" (MarcoFalke)
Pull request description:
This fixes a bug found with #18531:
* Currently the named argument for `generateblock` is documented as `address/descriptor`, but the server only accepts a named argument of `address`. Fix it by changing the name to `output` for both the documentation and the server code. Also, add tests to prove the server understands the new name `output`.
* Unrelated to that, there have been a bunch of aliases in the server code that are not present in the source code of the documentation. Fix that by adding the alias to the source code of the documentation. Only the first alias is displayed in the rendered documentation. Also, add tests to prove the server actually understands all aliases.
ACKs for top commit:
pierreN:
Tested ACK fa168d7 tests, help messages
Tree-SHA512: 05e15628e3a667b296f3783d20f764b450b959451b5360c7eaf5993156582d47a0f5882330ca2493b851eb46324d504953b90c875bc88a15c9e8c89eb3ef8d92
0753efd9dc rpc: Remove deprecated "size" from mempool txs (Vasil Dimov)
Pull request description:
Remove the "size" property of a mempool transaction from RPC replies.
Deprecated in e16b6a718 in 0.19, about 1 year ago.
ACKs for top commit:
kristapsk:
ACK 0753efd9dc
Tree-SHA512: 392ced6764dd6a1d47c6d1dc9de78990cf3384910d801253f8f620bd1751b2676a6b52bee8a30835d28e845d84bfb37431c1d2370f48c9d4dc8e6a48a5ae8b9e
c9017ce3bc protect g_chainman with cs_main (James O'Beirne)
2b081c4568 test: add basic tests for ChainstateManager (James O'Beirne)
4ae29f5f0c use ChainstateManager to initialize chainstate (James O'Beirne)
5b690f0aae refactor: move RewindBlockIndex to CChainState (James O'Beirne)
89cdf4d569 validation: introduce unused ChainstateManager (James O'Beirne)
8e2ecfe249 validation: add CChainState.m_from_snapshot_blockhash (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 introduces `ChainstateManager`, which is responsible for creating and managing access to multiple chainstates. Until we allow chainstate creation from UTXO snapshots (next assumeutxo PR?) it's basically unnecessary, but it is a prerequisite for background IBD support.
Changes are also made to the initialization process to make use of `g_chainman` and thus clear the way for multiple chainstates being loaded on startup.
One immediate benefit of this change is that we no longer have the `g_blockman` global, but instead have the ChainstateManager inject a reference of its shared BlockManager into any chainstate it creates.
Another immediate benefit is that uses of `ChainActive()` and `ChainstateActive()` are now covered by lock annotations. Because use of `g_chainman` is annotated to require cs_main, these two functions subsequently follow.
Because of whitespace changes, this diff looks bigger than it is. E.g., 4813167d98 is most easily reviewed with
```sh
git show --color-moved=dimmed_zebra -w 4813167d98
```
ACKs for top commit:
MarcoFalke:
re-ACK c9017ce3bc📙
fjahr:
Code Review Re-ACK c9017ce3bc
ariard:
Code Review ACK c9017ce
ryanofsky:
Code review ACK c9017ce3bc. No changes since last review other than a straight rebase
Tree-SHA512: 3f250d0dc95d4bfd70852ef1e39e081a4a9b71a4453f276e6d474c2ae06ad6ae6a32b4173084fe499e1e9af72dd9007f4a8a375c63ce9ac472ffeaada41ab508
fad691cafe rpc: Make verifychain default values static, not depend on global args (MarcoFalke)
Pull request description:
This fixes several issues:
* The documentation is not compile-time static and depends on run-time arguments, making it impossible to host it on a static resource like a website or pdf. See also a similar change in the wallet rpc code: #18499
* The same call (relying on default values) will run different code on different machines, depending on the command line args that were used to start the server. This might lead to hard-to-debug-remote issues.
This is a small behaviour change, and I will add release notes.
ACKs for top commit:
theStack:
ACK fad691cafe
promag:
Code review ACK fad691cafe.
Tree-SHA512: 1c7a253ff0ec13a973b10d3777b71c70954ded5805b65a3ab06317327014de4cd0601d71d30c6ce89a581722c150cb5567acc1bd3e0c789cb51bab6ef0dcfc4a
Fixes the following RPCExamples:
-> ExampleCli waitforblock (removed comma between arguments)
-> ExampleCli waitforblockheight (removed comma between arguments)
-> ExampleRpc waitforblockheight (removed quotation marks around integer argument)
fac52253f8 rpc: Document an RPCResult for all calls; Enforce at compile time (MarcoFalke)
fadd99f610 rpc: Add missing newline in RPCResult description (MarcoFalke)
Pull request description:
This documents the RPC Result (type and description, if applicable) everywhere it was missing. The patch can be reviewed with the `git diff` option `-W`/`--function-context`.
Also, code won't compile without having an RPCResult documented.
ACKs for top commit:
laanwj:
Lightly tested ACK fac52253f8
promag:
Tested ACK fac52253f8, built and verified listunspent help output.
Tree-SHA512: af2c1af1432beb944993776026c320814bfaecaf202f47359f5758849096ca7051ec6560395a2cc6678dcc111e7c9cf4917d0f0b221bdcf3ed1642e14d0e5b3c
fa6b061fc1 rpc: Auto-format RPCResult (MarcoFalke)
fa7d0503d3 rpc: Move OuterType enum to header (MarcoFalke)
Pull request description:
This enforces most syntax rules of the RPCResult at compile time (or some at run time during unit and functional tests)
Apart from normalizing the syntax, by separating stylistic formatting from the structure, we could in theory directly generate the html for e.g. https://bitcoincore.org/en/doc/0.19.0/rpc/wallet/importmulti/
ACKs for top commit:
Sjors:
Indeed, re-ACK fa6b061fc1
ajtowns:
ACK fa6b061fc1 -- skimmed code changes and differences to rpc help output
Tree-SHA512: 5b510b3aa0b7c7b9189a48c77593159409069f939145b9a00c5478e894cf65f994d44d633eb7bb7dbea40ee820645a2930976c24772379d96929002b120efa28
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
