fad943821e scripted-diff: Rename touched member variables (MarcoFalke)
fa663a4c0d Use mockable time for peer connection time (MarcoFalke)
fad7ead146 refactor: Use type-safe std::chrono in net (MarcoFalke)
Pull request description:
Benefits:
* Type-safe
* Mockable
* Allows to revert a temporary test workaround
ACKs for top commit:
naumenkogs:
ACK fad943821e
shaavan:
ACK fad943821e
Tree-SHA512: af9bdfc695ab727b100c6810a7289d29b02b0ea9fa4fee9cc1f3eeefb52c8c465ea2734bae0c1c63b3b0d6264ba2c493268bc970ef6916570eb166de77829d82
faa6c3d44c net: Drop only invalid entries when reading banlist.json (MarcoFalke)
Pull request description:
All entries will be dropped when there is at least one invalid one in `banlist.json`. Fix this by only dropping invalid ones.
Also suggested in https://github.com/bitcoin/bitcoin/pull/20966#issuecomment-861150204
ACKs for top commit:
laanwj:
Re-ACK faa6c3d44c
Tree-SHA512: 5a58e7f1dcabf78d0c65d8c6d5d997063af1efeaa50ca7730fc00056fda7e0061b6f7a38907ea045fe667c9f61d392e01e556b425a95e6b126e3c41cd33deb83
27f353d8ef build, android: Fix Android GUI not loading on Qt 5.15 (Igor Cota)
6fc5c772d4 build, qt: use static QAndroidPlatformIntegrationPlugin (Igor Cota)
Pull request description:
PR moved from https://github.com/bitcoin-core/gui/pull/504 as it escaped the confines of `src/qt`.
ACKs for top commit:
hebasto:
re-ACK 27f353d8ef
promag:
utACK 27f353d8ef
Tree-SHA512: 4b6e6b2fb1923b89934f11caa8c05c6f340881689273f0c08916144e623f03fd5b781f1a53af83f6e87dce211fe02a1cb87e5943d13811c791cc8aa458184d9f
2283b9cd1e test: add tests for LoadBlockIndex when using multiple chainstates (James O'Beirne)
0fd599a51a validation: have LoadBlockIndex account for snapshot use (James O'Beirne)
d0c6e61f5d validation: don't modify genesis during snapshot load (James O'Beirne)
Pull request description:
This is part of the [assumeutxo project](https://github.com/bitcoin/bitcoin/projects/11) (parent PR: #15606)
---
Currently, `BlockManager::LoadBlockIndex` adds all blocks that have downloaded transactions to the active chain state's `setBlockIndexCandidates` set, ignoring the background chain state.
This PR changes ChainstateManager::LoadBlockIndex to update `setBlockIndexCandidates` in the background chain, not just the active chain. In the active chain, the same blocks are added as before. In the background chain, only blocks that have actually been validated, not blocks marked assumed-valid are added so the background chain will continue to download and validate assumed-valid blocks.
ACKs for top commit:
MarcoFalke:
Concept ACK 2283b9cd1e🤽
Sjors:
utACK 2283b9cd1e
Tree-SHA512: 7c9a80802df4722d85d12b78d2e7f628ac5f11cb8be66913d5c3230339bd1220c6723805509d4460826a17d1dc04b0ae172eb7d09ac0ea5dc5e41d77975cbd5e
fa5865a9e3 Reduce size of strencodings decode tables (MarcoFalke)
fad6761cf7 Fix implicit integer sign changes in strencodings (MarcoFalke)
Pull request description:
This removes casts where they are nonsensical (ToUpper/ToLower) and adds them where needed. Then, remove the suppressions.
Also, reduce static memory usage.
ACKs for top commit:
shaavan:
crACK fa5865a9e3
Tree-SHA512: 29850fb616789befad17f71731f322b2238e94ec431cda293629de540f7ab02060a9fc5b7666168ca2592048df5a39a2975177f47771d4d8211d90321052549d
caac999ff0 refactor: remove dependence on AddrManTest (josibake)
f961c477b5 refactor: check Good() in tried_collisions test (josibake)
207f1c825c refactor: make AddrMan::Good return bool (josibake)
Pull request description:
Previously, the `addrman_tried_collisions` test behaved in the following way:
1. add an address to addrman
2. attempt to move the new address to the tried table (using `AddrMan.Good()`)
3. verify that `num_addrs` matched `size()` to check for collisions in the new table
`AddrMan.size()`, however, returns the number of unique address in addrman, regardless of whether they are in new or tried. This means the test would still pass for addresses where a collision did occur in the tried table. After 3 collisions in the tried table, there would eventually be a collision in the new table when trying to add a new address, which was then detected by checking `num_addrs - collisions == size()`.
While the collision in the new table was caused by a collision in the tried table, the test is misleading as it's not directly testing for collisions in the tried table and misses 3 collisions before identifying a collision in the new table.
### solution
To more directly test the tried table, I refactored `AddrMan::Good()` to return a boolean after successfully adding an address to the tried table. This makes the test much cleaner by first adding an address to new, calling `Good` to move it to the tried table, and checking if it was successful or not. It is worth noting there are other reasons, aside from collisions, which will cause `Good` to return false. That being said, this is an improvement over the previous testing methodology.
Additionally, having `Good()` return a boolean is useful outside of testing as it allows the caller to handle the case where `Good` is unable to move the entry to the tried table (e.g a063647413/src/rpc/net.cpp (L945)).
### followup
As a follow up to this PR, I plan to look at the following places `Good()` is called and see if it makes sense to handle the case where it is unable to add an entry to tried:
* a063647413/src/rpc/net.cpp (L945)
* a063647413/src/net.cpp (L2067)
* a063647413/src/net_processing.cpp (L2708)
ACKs for top commit:
jnewbery:
utACK caac999ff0
mzumsande:
Code review ACK caac999ff0
Tree-SHA512: f328896b1f095e8d2581fcdbddce46fc0491731a0440c6fff01081fa5696cfb896dbbe1d183eda2c100f19aa111e1f8b096ef93582197edc6b791de563a58f99
eaf6be0114 [net processing] Do not request transaction relay from feeler connections (John Newbery)
0220b834b1 [test] Add testing for outbound feeler connections (John Newbery)
Pull request description:
Feelers are short-lived connections used to test the viability of peers. The bitcoind node will periodically open feeler connections to addresses in its addrman, wait for a `version` message from the peer, and then close the connection.
Currently, we set `fRelay` to `1` in the `version` message for feeler connections, indicating that we want the peer to relay transactions to us. However, we close the connection immediately on receipt of the `version` message, and so never process any incoming transaction announcements. This PR changes that behaviour to instead set `fRelay` to `0` indicating that we do not wish to receive transaction announcements from the peer.
This PR also extends the `addconnection` RPC to allow creating outbound feeler connections from the node to the test framework, and a test to verify that the node sets `fRelay` to `0` in the `version` message to feeler connections.
ACKs for top commit:
naumenkogs:
ACK eaf6be0114
MarcoFalke:
review ACK eaf6be0114🏃
Tree-SHA512: 1c56837dbd0a396fe404a5e39f7459864d15f666664d6b35ad109628b13158e077e417e586bf48946a23bd5cbe63716cb4bf22cdf8781b74dfce6047b87b465a
Rather than try to infer a collision by checking `AddrMan::size`,
check whether or not moving to the tried table was successful by
checking the output from `AddrMan::Good`
If AddrMan::Good is unable to add an entry
to tried (for a number of reasons), return false.
This makes it much easier and cleaner to directly
test for tried collisions. It also allows anyone
calling Good() to handle the case where adding an
address to tried is unsuccessful.
Update docs to doxygen style.
fa19bab90a fuzz: Rework FillNode (MarcoFalke)
fae6e31df7 refactor: Set fSuccessfullyConnected in FillNode (MarcoFalke)
fa3583f856 fuzz: Avoid negative NodeId in ConsumeNode (MarcoFalke)
Pull request description:
Currently `FillNode` is a bit clumsy because it directly modifies memory of `CNode`. This gets in the way of moving that memory to `Peer`. Also, it isn't particularly consistent. See for example https://github.com/bitcoin/bitcoin/pull/21160#discussion_r739206139 .
Fix all issues by sending a `version`/`verack` in `FillNode` and let net_processing figure out the internal details.
ACKs for top commit:
jnewbery:
Strong concept ACK and light code review ACK fa19bab90a
Tree-SHA512: 33261d857c3fa6d5d39d742624009a29178ad5a15eb3fd062da741affa5a4854fd45ed20d59a6bba2fb068cf7b39cad6f95b2910be7cb6afdc27cd7917955b67
fa26c55644 wallet: Replace Assume with Assert where needed in coinselection (MarcoFalke)
Pull request description:
`Assume` should only be used when a failed check is recoverable. The checks here don't recover and would run into UB, so use `Assert` instead.
ACKs for top commit:
theStack:
Code-review ACK fa26c55644
Tree-SHA512: 0cf9435f9ec44794022ce0274cba602aec95102ab73f4c8a93dae54ef4c0a594f6a81640477039719ddfb6f23b05f8ece3e4886ef7f8a725efff45685ac49d92
Ensure that blocks past the snapshot base block (i.e. the end of the
assumed-valid region of the chain) are not included in
setBlockIndexCandidates for the background validation chainstate. These
blocks, while fully validated and lacking the BLOCK_ASSUMED_VALID flag,
*rely* on blocks which are assumed-valid, and so shouldn't be added to
the IBD chainstate.
Co-authored-by: Russ Yanofsky <russ@yanofsky.org>
Avoid modifying the genesis block index entry during snapshot load. This
is because, in a future change that fixes LoadBlockIndex for UTXO
snapshots, we detect block index entries that are reliant on
assumed-valid ancestors and treat them specially.
Since the genesis block doesn't have BLOCK_VALID_SCRIPTS, it would be
erroneously marked BLOCK_ASSUMED_VALID during snapshot load if we didn't
skip it here. This would cause a "setBlockIndexCandidates() empty"
assertion to be tripped since all block index entries would be marked
assume-valid due to genesis, which is never re-validated.
There's probably no good reason to modify the genesis block index entry
during snapshot load anyway...
fa24a3df87 rpc: Quote user supplied strings in error messages (MarcoFalke)
Pull request description:
I can't see a downside doing this and this fixes a fuzzing crash
Background:
This is a follow-up to commit 926fc2a0d4, which introduced the "starts_with-hack". Maybe an alternative to the hack would be to assign a unique error code to internal bugs? However, I think this can be done in an separate pull request and the changes here make sense even on their own.
ACKs for top commit:
fanquake:
ACK fa24a3df87 - to fix the fuzzers.
Tree-SHA512: d998626406a64396a037a6d1fce22fce3dadb7567c2f9638e450ebe8fb8ae77d134e15dd02555326732208f698d77b0028bc62be9ceee9c43282b61fe95fccbd
e9440aeb5c build: use __SIZEOF_INT128__ for checking __int128 availability (fanquake)
Pull request description:
We already use this in the blockfilter code,
bf66e258a8/src/blockfilter.cpp (L34-L36)
so not sure we need to maintain two different ways of testing
for the same functionality. Consolidate on testing for `__SIZEOF_INT128__`,
which we already use, is supported by the compilers we care about, and is
also used by libsecp256k1.
ACKs for top commit:
sipa:
utACK e9440aeb5c
Zero-1729:
crACK e9440aeb5c
Tree-SHA512: 8aeef1734486a863b5091123bb5f9ba8868b1e2b4b35114586e3eb5862a38d4a1518ed069f37f41cb5e5ce2f6c87d95671996366d5ee990e0c90f268a8978ba3
50209a42ad validation, doc: remove TODO comment (Jon Atack)
8e37fa8393 validation, log: improve logging in FlushSnapshotToDisk() (Jon Atack)
271252c0bd validation, log: extract FlushSnapshotToDisk() function (Jon Atack)
Pull request description:
Use the `LOG_TIME_MILLIS_WITH_CATEGORY_MSG_ONCE` macro to improve the logging of ChainstateManager snapshot persistance, log task start and completion separately and no longer manually track the duration, as suggested by Marco Falke in https://github.com/bitcoin/bitcoin/pull/22872#discussion_r715571280.
Extract the flushing into one function, which clarifies the logic, extends the improved logging to both flushing call sites, and allows logging the prefix `FlushSnapshotToDisk`, which is similar to `FlushStateToDisk`.
before
```
[snapshot] flushing coins cache (0 MB)... done (0.00ms)
[snapshot] flushing snapshot chainstate to disk
```
after
```
FlushSnapshotToDisk: flushing coins cache (0 MB) started
...
FlushSnapshotToDisk: completed (0.00ms)
FlushSnapshotToDisk: saving snapshot chainstate (0 MB) started
...
FlushSnapshotToDisk: completed (0.00ms)
```
The logging can be observed in the output of
```
./src/test/test_bitcoin -t validation_chainstate_tests -- DEBUG_LOG_OUT
```
Top commit has no ACKs.
Tree-SHA512: 5d954cd8c7455f8625152a43663a237f04717bb834aed62925a56e17c711fca6ccfc03783970b6b0bde44f64617d804b423a7048287c06ee816db36247acf272
We already use this in the blockfilter code, so not sure we need to maintain two
different ways of testing for the same functionality. Consolidate on testing
for __SIZEOF_INT128__, which we already use, is supported by the compilers we
care about, and is also used by libsecp256k1.
9600ea0145 test: Add edge case of pruning up to index height (Martin Zumsande)
698c524698 index: Fix backwards search for bestblock (Martin Zumsande)
Pull request description:
This PR attempts to fix an intermittent Init issue encountered during the stress testing of #23289, which relates to the pruning-compatible filter reconstruction logic introduced in #15946.
The problem would occur when the node starts with `-txindex=1` but `ThreadSync` is interrupted after it sets `m_best_block_index` to Genesis, and before it gets do any further work.
In that case, during the next restart of the node, an Init error would be thrown because `BaseIndex::Init()` tries to backtrack from the tip to the last block which has been successfully indexed (here: Genesis), but the backtracking logic didn't work properly in this case:
The loop
`while (block_to_test && block->pprev && (block->pprev->nStatus & BLOCK_HAVE_DATA))` checks if a predecessor exists **before** performing the check `block_to_test == block` and then possbily setting `prune_violation = false`
If `block_to_test` and `block` are the Genesis block this check will not be reached because `block->pprev` does not exist.
To reproduce this bug on regtest:
1) start a node with a fresh datadir using `-txindex=1` (or any other index)
2) stop and restart without any index
3) mine a block
3) stop and restart again with the index enabled
->InitError `Error: txindex best block of the index goes beyond pruned data. (...)`
Fix this by requiring that we have the data for the block of the current iteration `block` (instead of requiring it for the predecessor `block->pprev`)
That way, the check for `block_to_test == block` is also reached when `block_to_test` is the Genesis block.
No longer requiring the data of `block->pprev` also means that we can now prune up to `m_best_block_index` height without requiring a reindex (one block more than before). I added this edge case to `feature_blockfilterindex_prune.py`, the new version should fail on master.
ACKs for top commit:
ryanofsky:
Partial code review ACK 9600ea0145 for the code change, not the test changes. (Test changes are indirect and little over my head.) It seems obvious that previous code `prune_violation = true, while (block->pprev)` would incorrectly detect a prune violation at the genesis block, and the fix here make sense and looks correct.
Tree-SHA512: c717f372cee8fd49718b1b18bfe237aa6ba3ff4468588c10e1272d7a2ef3981d10af4e57de51dec295e2ca72d441bc6c2812f7990011a94d7f818775e3ff1a38
It would make for sense for the TODO to be done in PR 17487
(or noted in the review feedback for a follow-up),
no need to continue maintaining the TODO in the codebase.
d5cab1a96d Add createmultisig and addmultisigaddress warnings release note (Samuel Dobson)
e46fc935aa Add warnings field to addmultisigaddress to warn about uncompressed keys (Samuel Dobson)
d1a9742623 Add warnings field to createmultisig to warn about uncompressed keys (Samuel Dobson)
Pull request description:
Fixes#21368
Currently, if there are any uncompressed keys when calling `AddAndGetMultisigDestination`, it will just default to a legacy address regardless of the chosen `address_type`. Rather than keeping this silent behaviour which may be confusing to users, we explicitly add a `warnings` field which will warn the user why their address format is different.
ACKs for top commit:
achow101:
ACK d5cab1a96d
Tree-SHA512: c2ac7f7689251bd4fcd8c26506f053921fbaf34c7a26a74e82ebc7f82cc0bd25407fd7954bf98365dcafa51fa45dcdbee6214320580ca69509690c3555e71cc0
2f97c1180b doc: Remove TODO 'exclude peers with download permission' (Douglas Chimento)
Pull request description:
Following from PR https://github.com/bitcoin/bitcoin/pull/23109
The [TODO](https://github.com/bitcoin/bitcoin/blob/master/src/net.cpp#L2872) is no longer necessary.
Removing it to prevent future confusion.
Top commit has no ACKs.
Tree-SHA512: c2f4c3eae951d13d623e1b4bd9315804ec33473e501367f89edae80fa446674edc71549e145f058fe7126b8588790dc895d6ea3dfb4347ceeca61f5e5f2f95cc
fa72dd314f fuzz: Move ISO8601 to one place (MarcoFalke)
Pull request description:
Seems confusing to split this to two places.
Also fixes https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=42178
ACKs for top commit:
fanquake:
ACK fa72dd314f
Tree-SHA512: 637b0671078848ea417fdf66b92715602040fad34d4ca5f7b843a519a1cfeebe5d992a79a399deba39926905125681d66ab0dc05f66f79a26f3bf555e12fb0ba
81521173ba Merge global xpubs in joinpsbts and combinepsbts (Andrew Chow)
d8043ddf64 Add global xpub test vectors from BIP (Andrew Chow)
35670df866 Add global_xpubs to decodepsbt (Andrew Chow)
903848562e Implement serializations for PSBT_GLOBAL_XPUB (Andrew Chow)
c5c63b8e4f Implement operator< for KeyOriginInfo and CExtPubKey (Andrew Chow)
d3dbb16168 Separate individual HD Keypath serialization into separate functions (Andrew Chow)
a69332fd89 Store version bytes and be able to serialize them in CExtPubKey (Andrew Chow)
5fdaf6a2ad moveonly: Move (Un)Serialize(To/From)Vector, (De)SerializeHDKeypaths to psbt module (Andrew Chow)
94065cc6c5 Test for proprietary field (Andrew Chow)
a4cf810174 Output proprietary type info in decodepsbt (Andrew Chow)
aebe758e54 Implement PSBT proprietary type (Andrew Chow)
10ba0b593d Output psbt version in decodepsbt (Andrew Chow)
df84fa99c5 Add GetVersion helper to PSBT (Andrew Chow)
c3eb416b88 Implement PSBT versions (Andrew Chow)
3235847473 Types are compact size uints (Andrew Chow)
Pull request description:
Implements the changes to BIP 174 proposed in https://github.com/bitcoin/bips/pull/849 and https://github.com/bitcoin/bips/pull/784
Implements `PSBT_GLOBAL_VERSION`, `PSBT_GLOBAL_PROPRIETARY`, `PSBT_IN_PROPRIETARY`, `PSBT_OUT_PROPRIETARY`, and `PSBT_GLOBAL_XPUB`. The `PSBT_GLOBAL_XPUB` changes are merged in from #16463.
Also includes the test vectors added to BIP 174 for these fields.
A number of additional changes to keypath and xpub serialization are made to support `PSBT_GLOBAL_XPUB`.
ACKs for top commit:
laanwj:
Code review ACK 81521173ba
Tree-SHA512: bd71c3f26030fc23824e76a30d3d346a753e1db224ecee163d6813348feb52d3f4cf4e739a4699e2cff381197ce2a7ea4a92a054f2c3e1db579e91e92a0945e0
Use the `LOG_TIME_MILLIS_WITH_CATEGORY_MSG_ONCE` macro to improve the
logging of snapshot persistance and no longer manually track the duration.
before
[snapshot] flushing coins cache (0 MB)... done (0.00ms)
[snapshot] flushing snapshot chainstate to disk (0 MB)... done (0.00ms)
after
FlushSnapshotToDisk: flushing coins cache (0 MB) started
FlushSnapshotToDisk: completed (0.00ms)
FlushSnapshotToDisk: saving snapshot chainstate (0 MB) started
FlushSnapshotToDisk: completed (0.00ms)
The logging can be observed in the output of
./src/test/test_bitcoin -t validation_chainstate_tests -- DEBUG_LOG_OUT
7f15eff2dd style-only: Remove redundant scope in *Chainstate (Carl Dong)
89bec827fd Collapse the 2 cs_main locks in LoadChainstate (Carl Dong)
3b1584b794 Remove all #include // for * comments (Carl Dong)
9a5a5a3d08 test/setup: Use LoadChainstate (Carl Dong)
c541da0d62 node/chainstate: Add options for in-memory DBs (Carl Dong)
ceb9790341 node/caches: Remove intermediate variables (Carl Dong)
ac4bf138b8 node/caches: Extract cache calculation logic (Carl Dong)
15f2e33bb3 validation: VerifyDB only needs Consensus::Params (Carl Dong)
4da9c076d1 node/chainstate: Decouple from ShutdownRequested (Carl Dong)
05441c2dc5 node/chainstate: Decouple from GetTime (Carl Dong)
2414ebc18b init: Delay RPC block notif until warmup finished (Carl Dong)
8d466a8504 Move -checkblocks LogPrintf to AppInitMain (Carl Dong)
aad8d59789 node/chainstate: Reduce coupling of LogPrintf (Carl Dong)
b345979a2b node/chainstate: Decouple from concept of uiInterface (Carl Dong)
ca7c0b934d Split off VerifyLoadedChainstate (Carl Dong)
adf4912d77 node/chainstate: Remove do/while loop (Carl Dong)
975235ca0a Move init logistics message for BAD_GENESIS_BLOCK to init.cpp (Carl Dong)
8715658983 Move mempool nullptr Assert out of LoadChainstate (Carl Dong)
9162a4f93e node/chainstate: Decouple from concept of NodeContext (Carl Dong)
c7a5c46e6f node/chainstate: Decouple from ArgsManager (Carl Dong)
ae9121f958 node/chainstate: Decouple from stringy errors (Carl Dong)
cbac28b72f node/chainstate: Decouple from GetTimeMillis (Carl Dong)
cb64af9635 node: Extract chainstate loading sequence (Carl Dong)
Pull request description:
This PR:
1. Coalesce the Chainstate loading sequence between `AppInitMain` and `*TestingSetup` (which makes it more tested)
2. Makes the Chainstate loading sequence reusable in preparation for future work extracting out our consensus engine.
Code-wise, this PR:
1. Extracts `AppInitMain`'s Chainstate loading sequence into a `::LoadChainstateSequence` function
2. Makes this `::LoadChainstateSequence` function reusable by
1. Decoupling it from various concepts (`ArgsManager`, `uiInterface`, etc)
2. Making it report errors using an `enum` rather than by setting a `bilingual_str`
3. Makes `*TestingSetup` use this new `::LoadChainstateSequence`
Reviewers: Aside from commentary, I've also included `git diff` flags of interest in the commit messages which I hope will aid review!
ACKs for top commit:
ryanofsky:
Code review ACK 7f15eff2dd. Thanks for updates!
MarcoFalke:
review ACK 7f15eff2dd💳
Tree-SHA512: fb9a6cbd1c511a52b477c62a5e68e53a8be5dec2fff0e44a279966afb91efbab44bf1fe7c6b1519f8464ecc25f42dd4bae8e1efbf55ee91fc90fa0b92e3a83e2
