4523d28b6b [test] compare filter and header with the result of the getblockfilter RPC (Niklas Gögge)
3a2464f216 [rest] drop superfluous rpc serializations flags for block filters (Niklas Gögge)
064abd14a5 [rest] add a more verbose error message for invalid header counts (Niklas Gögge)
83b8f3a896 [refactor] various style fix-ups (Niklas Gögge)
Pull request description:
This PR addresses unresolved review comments from [#17631](https://github.com/bitcoin/bitcoin/pull/17631).
This includes:
* various style fix-ups
* returning a more verbose error message for invalid header counts
* removing superfluous rpc serializations flags for block filters
* improving the test to include comparing the block filters returned from the rest with the ones returned from the `getblockfilter` RPC.
ACKs for top commit:
jnewbery:
ACK 4523d28b6b
brunoerg:
tACK 4523d28b6b
Tree-SHA512: 634e6b2ae3e1d5f31675a50cfe11a5e74bf5a51b9e7b512d9e18879bf2ed424fc0ac6ec633023f192e3ad12cf0c73b0b51de79dd7ec00844dba3e1493d823b8c
2b64fa3251 Update REST docs with new accessors (Matt Corallo)
ef7c8228fd Expose block filters over REST. (Matt Corallo)
Pull request description:
This adds a new rest endpoint:
/rest/blockfilter/filtertype/requesttype/blockhash (eg
/rest/blockfilter/basic/header/000000000019d6689c085ae165831e934ff763ae46a2a6c172b3f1b60a8ce26f.hex)
which exposes either the filter "header" or the filter data itself.
Most of the code is cribbed from the equivalent RPC.
You can test it at http://bitcoin-rest.bitcoin.ninja/rest//blockfilter/basic/header/000000005b7a58a939b2636f61fa4ddd62258c5fed57667a35d23f2334c4f86d.hex
ACKs for top commit:
dergoegge:
ACK 2b64fa3251 - Adding blockfilters to the REST interface is analogous to serving other public data such as transactions or blocks.
Tree-SHA512: d487bc694266375c94d6fcf2e9d788a8a42a3b94e8d3290e46335a64cbcde55084ce5ea6119b79a4065888d94d7c3ae25a59a901fa46e3711f0eb296add12696
5c34507ecb core_write: Rename calculate_fee to have_undo for clarity (fyquah)
8edf6204a8 release-notes: Add release note about getblock verbosity level 3. (fyquah)
459104b2aa rest: Add test for prevout fields in getblock (fyquah)
4330af6f72 rpc: Add test for level 3 verbosity getblock rpc call. (fyquah)
51dbc167e9 rpc: Add level 3 verbosity to getblock RPC call. (fyquah)
3cc95345ca rpc: Replace boolean argument for tx details with enum class. (fyquah)
Pull request description:
Author of #21245 expressed [time issues](https://github.com/bitcoin/bitcoin/pull/21245#issuecomment-902332088) in the original PR. Given that #21245 has received a lot of review*, I have decided to open this new pull request with [modifications required to get ACK from luke-jr ](https://github.com/bitcoin/bitcoin/pull/21245#issuecomment-905150806) and a few nits of mine.
### Original PR description
> Display the prevout in transaction inputs when calling getblock level 3 verbosity. This PR affects the existing `/rest/block` API by adding a `prevout` fields to tx inputs. This is mentioned in the change to the release notes.
>
> I added some functional tests that
>
> * checks that the RPC call still works when TxUndo can't be found
>
> * Doesn't display the "value" or "scriptPubKey" of the previous output when at a lower verbosity level
>
>
> This "completes" the issue #18771
### Possible improvements
* b0bf4f255f - I can include even this commit to this PR if deemed useful or I can leave it for a follow-up PR. See https://github.com/bitcoin/bitcoin/pull/21245#issuecomment-894853784 for more context.
### Examples
Examples of the `getblock` output with various verbose levels. Note that `000000000000001f682b188971cc1a121546be4e9d5baf22934fdc7f538288d5` contains only 2 transactions.
#### Verbose level 0
```bash
./bitcoin-cli -testnet getblock 000000000000001f682b188971cc1a121546be4e9d5baf22934fdc7f538288d5 0
```
##### Verbose level 1
```bash
./bitcoin-cli -testnet getblock 000000000000001f682b188971cc1a121546be4e9d5baf22934fdc7f538288d5 1
```
##### Verbose level 2
```bash
./bitcoin-cli -testnet getblock 000000000000001f682b188971cc1a121546be4e9d5baf22934fdc7f538288d5 2
```
##### Verbose level 3
```bash
./bitcoin-cli -testnet getblock 000000000000001f682b188971cc1a121546be4e9d5baf22934fdc7f538288d5 3
```
#### REST
```bash
curl -H "content-type:text/plain;" http://127.0.0.1:18332/rest/block/000000000000001f682b188971cc1a121546be4e9d5baf22934fdc7f538288d5.json
```
<sub>* ... and my everyday obsessive checking of my email inbox whether the PR moves forward.</sub>
Edit laanwj: Removed at symbol from message, and large example output to prevent it from all ending up in the commit message.
ACKs for top commit:
0xB10C:
ACK 5c34507ecb
meshcollider:
utACK 5c34507ecb
theStack:
ACK 5c34507ecb👘
promag:
Concept ACK 5c34507ecb
Tree-SHA512: bbff120d8fd76e617b723b102b0c606e0d8eb27f21c631d5f4cdab0892137c4bc7c65b1df144993405f942c91be47a26e80480102af55bff22621c19f518aea3
This adds a new rest endpoint:
/rest/blockfilter/filtertype/requesttype/blockhash (eg
/rest/blockfilter/basic/header/000000000019d6689c085ae165831e934ff763ae46a2a6c172b3f1b60a8ce26f.hex)
which exposes either the filter "header" or the filter data itself.
Most of the code is cribbed from the equivalent RPC.
Display the prevout in transaction inputs when calling getblock level 3
verbosity.
Co-authored-by: Luke Dashjr <luke_github1@dashjr.org>
Co-authored-by: 0xB10C <19157360+0xB10C@users.noreply.github.com>
This is not the cleanest change but:
1. It fixes the erroneous use of RPC's Ensure*() in rest.cpp, which
cause crashes in REST contexts.
RPC code wraps all calls in a try/except, REST code does not.
Ensure*(), being part of RPC, expects that its throw's will get
caught by a try/except. But if you use Ensure*() in REST code, since
it doesn't have a try/except wrap, a crash will happen.
2. It is consistent with other functions like GetMemPool.
Someone can probably make this a bit prettier.
586190f0b4 rpc/rest: Take and reuse local Chain/ChainState obj (Carl Dong)
bc3bd36902 rpc: style: Improve BuriedForkDescPushBack signature (Carl Dong)
f99913969f rpc: Remove unnecessary casting of block height (Carl Dong)
6a3d192020 rpc: Tidy up local references (see commit message) (Carl Dong)
038854f31e rest/rpc: Remove now-unused old Ensure functions (Carl Dong)
6fb65b49f4 scripted-diff: rest/rpc: Use renamed EnsureAny*() (Carl Dong)
1570c7ee98 rpc: Add renamed EnsureAny*() functions (Carl Dong)
306b1cd3ee rpc: Add alt Ensure* functions acepting NodeContext (Carl Dong)
d7824acdb9 rest: Use existing NodeContext (Carl Dong)
3f08934799 rest: Pass in NodeContext to rest_block (Carl Dong)
7be0671b95 rpc/rawtx: Use existing NodeContext (Carl Dong)
60dc05afc6 rpc/mining: Use existing NodeContext (Carl Dong)
d485e815e2 rpc/blockchain: Use existing NodeContext (Carl Dong)
d0abf0bf42 rpc/*,rest: Add review-only assertion to EnsureChainman (Carl Dong)
cced0f46c9 miner: Pass in previous CBlockIndex to RegenerateCommitments (Carl Dong)
Pull request description:
Overall PR: #20158 (tree-wide: De-globalize ChainstateManager)
Based on:
- [x] #21270 | [Bundle 4/n] Prune g_chainman usage in validation-adjacent modules
- [x] #21525 | [Bundle 4.5/n] Followup fixups to bundle 4
Note to reviewers:
1. This bundle may _apparently_ introduce usage of `g_chainman` or `::Chain(state|)Active()` globals, but these are resolved later on in the overall PR. [Commits of overall PR](https://github.com/bitcoin/bitcoin/pull/20158/commits)
2. There may be seemingly obvious local references to `ChainstateManager` or other validation objects which are not being used in callers of the current function in question, this is done intentionally to **_keep each commit centered around one function/method_** to ease review and to make the overall change systematic. We don't assume anything about our callers. Rest assured that once we are considering that particular caller in later commits, we will use the obvious local references. [Commits of overall PR](https://github.com/bitcoin/bitcoin/pull/20158/commits)
3. When changing a function/method that has many callers (e.g. `LookupBlockIndex` with 55 callers), it is sometimes easier (and less error-prone) to use a scripted-diff. When doing so, there will be 3 commits in sequence so that every commit compiles like so:
1. Add `new_function`, make `old_function` a wrapper of `new_function`, divert all calls to `old_function` to `new_function` **in the local module only**
2. Scripted-diff to divert all calls to `old_function` to `new_function` **in the rest of the codebase**
3. Remove `old_function`
ACKs for top commit:
ryanofsky:
Code review ACK 586190f0b4. Since last review, no changes to existing commits, just some simple new commits added: three new commits renaming std::any Ensure functions (scripted diff commit and manual pre/post commits), and one new commit factoring out a repeated `ActiveChain()` call made in a loop. Thanks for the updates!
jnewbery:
utACK 586190f0b4
MarcoFalke:
review ACK 586190f0b4🍯
Tree-SHA512: 64b677fb50141805b55c3f1afe68fcd298f9a071a359bdcd63256d52e334f83e462f31fb3ebee9b630da8f1d912a03a128cfc38179e7aaec29a055744a98478c
In all rest/rpc-related modules, if there are multiple calls to
ActiveChain{,State}(), and the calls fall under the same ::cs_main lock,
we can simply take a local reference and use/reuse it instead of calling
ActiveChain{,State}() again and again.
Organize local variables/references such that:
1. There is always a `ChainstateManager` reference before any `LOCK(cs_main)`.
2. NodeContext references are used with Ensure*() functions introduced in
previous commit where appropriate to avoid duplicate assertions.
fadcd3f78e doc: Remove irrelevant link to GitHub (MarcoFalke)
fa121b628d blockstorage: [refactor] Use chainman reference where possible (MarcoFalke)
fa0c7d9ad2 move-only: Move *Disk functions to blockstorage (MarcoFalke)
fa91b2b2b3 move-only: Move AbortNode to shutdown (MarcoFalke)
fa413f07a1 move-only: Move ThreadImport to blockstorage (MarcoFalke)
faf843c07f refactor: Move load block thread into ChainstateManager (MarcoFalke)
Pull request description:
This picks up the closed pull request #21030 and is the first step toward fixing #21220.
The basic idea is to move all disk access into a separate module with benefits:
* Breaking down the massive files init.cpp and validation.cpp into logical units
* Creating a standalone-module to reduce the mental complexity
* Pave the way to fix validation related circular dependencies
* Pave the way to mock disk access for testing, especially where it is performance critical (like fuzzing)
ACKs for top commit:
promag:
Code review ACK fadcd3f78e, checked (almost) moved only changes. This is a nice tidy up change and doesn't change behavior. Easily reviewed commit by commit.
jamesob:
ACK fadcd3f78e ([`jamesob/ackr/21575.1.MarcoFalke.refactor_create_blocksto`](https://github.com/jamesob/bitcoin/tree/ackr/21575.1.MarcoFalke.refactor_create_blocksto))
ryanofsky:
Code review ACK fadcd3f78e. New organization makes sense, moves extraneous things outside of validation.cpp. PR is also easy to review with helpfully split up moveonly commits.
Tree-SHA512: 917996592b6d8f9998289d8cb2b1b78b23d1fdb3b07216c9caec1380df33baa09dc2c1e706da669d440b497e79c9c62a01ca20dc202df5ad974a75f3ef7a143b
e829c9afbf refactor: replace sizeof(a)/sizeof(a[0]) by std::size (C++17) (Sebastian Falbesoner)
365539c846 refactor: init vectors via std::{begin,end} to avoid pointer arithmetic (Sebastian Falbesoner)
63d4ee1968 refactor: iterate arrays via C++11 range-based for loops if idx is not needed (Sebastian Falbesoner)
Pull request description:
This refactoring PR picks up the idea of #19626 and replaces all occurences of `sizeof(x)/sizeof(x[0])` (or `sizeof(x)/sizeof(*x)`, respectively) with the now-available C++17 [`std::size`](https://en.cppreference.com/w/cpp/iterator/size) (as [suggested by sipa](https://github.com/bitcoin/bitcoin/pull/19626#issuecomment-666487228)), making the macro `ARRAYLEN` obsolete.
As preparation for this, two other changes are done to eliminate `sizeof(x)/sizeof(x[0])` usage:
* all places where arrays are iterated via an index are changed to use C++11 range-based for loops If the index' only purpose is to access the array element (as [suggested by MarcoFalke](https://github.com/bitcoin/bitcoin/pull/19626#discussion_r463404541)).
* `std::vector` initializations are done via `std::begin` and `std::end` rather than using pointer arithmetic to calculate the end (also [suggested by MarcoFalke](https://github.com/bitcoin/bitcoin/pull/20429#discussion_r567418821)).
ACKs for top commit:
practicalswift:
cr ACK e829c9afbf: patch looks correct
fanquake:
ACK e829c9afbf
MarcoFalke:
review ACK e829c9afbf 🌩
Tree-SHA512: b01d32c04b9e04d562b7717cae00a651ec9a718645047a90761be6959e0cc2adbd67494e058fe894641076711bb09c3b47a047d0275c736f0b2218e1ce0d193d
[META] In a previous commit, we moved ::LookupBlockIndex to become a
member function of BlockManager. This commit is split out from
that one since it can be expressed nicely as a scripted-diff.
-BEGIN VERIFY SCRIPT-
find_regex='LookupBlockIndex' \
&& git grep -l -E "$find_regex" -- src \
| grep -v '^src/validation\.\(cpp\|h\)$' \
| xargs sed -i -E "s@${find_regex}@g_chainman.m_blockman.LookupBlockIndex@g"
-END VERIFY SCRIPT-
