4d2fa97031 [addrman] Clean up ctor (John Newbery)
7e6e65918f [addrman] inline Clear() into CAddrMan ctor (John Newbery)
406be5ff96 [addrman] Remove all public uses of CAddrMan.Clear() from the tests (John Newbery)
ed9ba8af08 [tests] Remove CAddrMan.Clear() call from CAddrDB::Read() (John Newbery)
e8e7392311 [addrman] Don't call Clear() if parsing peers.dat fails (John Newbery)
181a1207ba [addrman] Move peers.dat parsing to init.cpp (John Newbery)
Pull request description:
`CAddrMan::Clear()` exists to reset the internal state of `CAddrMan`. It's currently used in two places:
- on startup, if deserializing peers.dat fails, `Clear()` is called to reset to an empty addrman
- in tests, `Clear()` is called to reset the addrman for more tests
In both cases, we can simply destruct the `CAddrMan` and construct a new, empty addrman. That approach is safer - it's possible that `Clear()` could 'reset' the addrman to a state that's not equivalent to a freshly constructed addrman (one actual example of this is that `Clear()` does not clear the `m_tried_collisions` set). On the other hand, if we destruct and then construct a fresh addrman, we're guaranteed that the new object is empty.
This wasn't possible when addrman was initially implemented, since it was a global, and so it would only be destructed on shutdown. However, addrman is now owned by `node.context`, so we have control over its destruction/construction.
ACKs for top commit:
laanwj:
Code review ACK 4d2fa97031
vasild:
ACK 4d2fa97031
Zero-1729:
crACK 4d2fa97031
Tree-SHA512: f715bf2cbff4f8c3a9dbc613f8c7f11846b065d6807faf3c7d346a0b0b29cbe7ce1dc0509465c2c9b88a8ad55299c9182ea53f5f743e47502a69a0f375e09408
Clear() is now only called from the ctor, so just inline the code into
that function.
The LOCK(cs) can be removed, since there can be no data races in the ctor.
Also move the function definition out of the header and into the cpp file.
Just use unique_ptr<CAddrMan>s and reset the pointer if a frest addrman is required.
Also make CAddrMan::Clear() private to ensure that no call sites are missed.
cd37356ff9 [crypto] Fix K1/K2 use in ChaCha20-Poly1305 AEAD (Dhruv Mehta)
Pull request description:
BIP324 mentions K1 is used for the associated data and K2 is used for the payload. The code does the opposite. This is not a security problem but will be a problem across implementations based on the HKDF key derivations.
BIP324 author Jonas Schnelli thinks a [code update will be better](https://github.com/bitcoin/bitcoin/pull/15649#discussion_r440780669) than a BIP update.
If this PR is merged:
- [ ] We need to update the test vector 3 in BIP324
ACKs for top commit:
jonasschnelli:
utACK cd37356ff9
Tree-SHA512: e2165117bfbf7a031060e7376912f9af1c1bfc57916383799a0fa2c040e2caaab0d6aafc3425c083a233b96c84fafec75c938e00ceb6bd7d52607d58607cb145
f12fbad5a1 windres: use PACKAGE_VERSION rather than building more version numbers (fanquake)
Pull request description:
Rather than defining more strings, reuse PACKAGE_VERSION, which is already available.
We also already use PACKAGE_VERSION for `ProductVersion` and `FileVersion` in setup.nsi.
ACKs for top commit:
MarcoFalke:
cr ACK f12fbad5a1
laanwj:
Code review ACK f12fbad5a1
Tree-SHA512: b74a37cbba105d208d4da9264d295d7e052009fdd6b0ed54a0d9968bbe2deeba1766d6d310438b2939a81555faa0cbd67d5e53f0c8a2de669ce56353c1c67d22
`bool CAddrDB::Read(CAddrMan& addr, CDataStream& ssPeers)` is _only_
called from the tests, and the call to addr.Clear() only exists so that
a test that Clear() is called passes. Remove that test and the call.
Now that we manage the lifespan of node.addrman, we can just reset
node.addrman to a newly initialized CAddrMan if parsing peers.dat
fails, eliminating the possibility that Clear() leaves some old state
behind.
aaa6ad5455 [MOVEONLY] [tests] Move addrman ser/deser tests to addrman_tests.cpp (John Newbery)
Pull request description:
Addrman serialization/deserialization tests are currently in net_tests.cpp.
Move them to addrman_tests.cpp with the rest of the addrman tests.
Reviewer hint: review using `git diff --color-moved=dimmed-zebra`
ACKs for top commit:
MarcoFalke:
review ACK aaa6ad5455📺
mjdietzx:
ACK aaa6ad5455
Saviour1001:
Concept ACK <code>[aaa6ad5](aaa6ad5455)</code>
Tree-SHA512: 9a575e63f8830a9dfba30ef63c83ae4b45813add9973308f0d7a2463886438575a2e8a4a23af5b19ed977fbaab1a212ef832f89a25de03920d493e9ebafa9c30
5c5d0b6264 Add FoundBlock.found member (Russell Yanofsky)
Pull request description:
This change lets IPC serialization code handle FoundBlock arguments more simply and efficiently. Without this change there was no way to determine from a FoundBlock object whether a block was found or not. So in order to correctly implement behavior of leaving FoundBlock output variables unmodified when a block was not found, IPC code would have to read preexisting output variable values from the local process, send them to the remote process, receive output values back from the remote process, and save them to output variables unconditionally. With FoundBlock.found method, the process is simpler. There's no need to read or send preexisting local output variable values, just to read final output values from the remote process and set them conditionally if the block was found.
---
This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/projects/10). The commit was first part of larger PR #10102.
ACKs for top commit:
fjahr:
Code review ACK 5c5d0b6264
theStack:
Concept and code review ACK 5c5d0b6264
jamesob:
ACK 5c5d0b6264 ([`jamesob/ackr/22215.1.ryanofsky.refactor_add_foundblock`](https://github.com/jamesob/bitcoin/tree/ackr/22215.1.ryanofsky.refactor_add_foundblock))
Zero-1729:
crACK 5c5d0b6
Tree-SHA512: d906e1b7100ff72c3aa06d80bd77673887b2db670ebd52dce7c4f6f557a23a1744c6109308228a37fda6c6ea74f05ba0efecff0ef235ab06ea8acd861fbb8675
Addrman serialization/deserialization tests are currently in net_tests.cpp.
Move them to addrman_tests.cpp with the rest of the addrman tests.
Reviewer hint: review using `git diff --color-moved=dimmed-zebra`
Rather than defining more strings, reuse PACKAGE_VERSION, which is
already available.
We already use PACKAGE_VERSION for `ProductVersion` and `FileVersion` in setup.nsi.
Use interfaces::Init::make* methods instead of interfaces::Make*
functions, so interfaces can be constructed differently in different
executables without having to change any code. (So for example
bitcoin-gui can make an interfaces::Node pointer that communicates with
a bitcoin-node subprocess, while bitcoin-qt can make an interfaces::Node
pointer that starts node code in the same process.)
60e0cbdd57 [addrman] Merge the two Add() functions (Amiti Uttarwar)
Pull request description:
This PR merges the two definitions of this overloaded function to reduce code duplication.
When these functions were introduced in 5fee401fe1, there were multiple places that invoked `Add()` with a single addr and a vector of addrs each, so it made sense to overload the function. I could see how the small difference in log statement was more meaningful when a peer was added via IRC :)
Now, the definition of `Add()` that takes in a single address is only invoked from the hidden/test-only RPC `addpeeraddress`. These changes should not cause any observable difference, and are covered by the existing tests that use this RPC endpoint.
ACKs for top commit:
jnewbery:
Code review ACK 60e0cbdd57
Zero-1729:
crACK 60e0cbd
fanquake:
ACK 60e0cbdd57
Tree-SHA512: 782fb2ac6d2d403ba7d7ff543197ca42b610b9a8806952d271e57e2ee3527ad1a94af4ebbad5371b5e95d77df07c56ccc8c1d5a2c82cdecb0d2b5085b3bdd5ee
2d7534bd93 wallet: use PACKAGE_NAME instead of "Bitcoin" in rpcdump (fanquake)
14b4802405 wallet: use FormatFullVersion instead of CLIENT_BUILD in rpcdump (fanquake)
Pull request description:
The dumpwallet RPC is the last place we're using CLIENT_BUILD directly, rather FormatFullVersion() (which just returns it), so switch to using that. At the same time, use PACKAGE_NAME (Bitcoin Core), rather than just "Bitcoin".
ACKs for top commit:
MarcoFalke:
cr ACK 2d7534bd93
laanwj:
Tested ACK 2d7534bd93
achow101:
ACK 2d7534bd93
Zero-1729:
crACK 2d7534b
Tree-SHA512: b38ee074e317448719d2a628380786ec665413515b38d9ce680c21608bc2acf6a2bf817f78f100a8310477613ae72d6969cc4f595f4f44af0896659d3ebf2671
fa7718344d fuzz: Avoid OOM in system fuzz target (MarcoFalke)
Pull request description:
If the inputs size is unlimited, the target may consume unlimited memory, because the argsmanager stores the argument names. Limiting the size should fix this issue.
Should fix https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=36906
ACKs for top commit:
practicalswift:
cr ACK fa7718344d
Tree-SHA512: 6edfcf324ee9d94e511038ee01340f02db50bcb233af3f1a1717c3602164c88528d9d987e971ec32f1a4593b868019bea0102c53c9b02bfefec3dfde959483cf
5100deee58 clientversion: No suffix #if CLIENT_VERSION_IS_RELEASE (Carl Dong)
Pull request description:
```
Previously, building from a release source tarball would result in a
version string like v22.0.0-<commithash>, but we expect just v22.0.0.
This commit solves this problem.
Also use PACKAGE_VERSION instead of reconstructing it.
```
Fixes the underlying problem of #22623
ACKs for top commit:
achow101:
ACK 5100deee58
fanquake:
ACK 5100deee58 - tested that prior the output of `src/bitcoind -version` on the `22.x` branch was `Bitcoin Core version v22.0.0-d3bd5410f64e`, and with this commit cherry-picked it is `Bitcoin Core version v22.0.0rc2`.
Tree-SHA512: 78705e285ff1271d5012e888837049044db4d11d66c252c6b964685892ef078c56fe122f12daa87c71532f4352f695d1e88a228665adcd7afe3ddce3f209b49f
92885c4f69 test: Test for ApproximateBestSubset edge case with too little fees (Andrew Chow)
d9262324e8 wallet: Assert that enough was selected to cover the fees (Andrew Chow)
2de222c401 wallet: Use GetSelectionAmount for target value calculations (Andrew Chow)
Pull request description:
The `m_value` used for the target calculation in `ApproximateBestSubset` is incorrect, it should be `GetSelectionAmount`. This causes a bug that is only apparent when the minimum relay fee is set to be very high.
A test case is added for this, in addition to an assert in `CreateTransactionInternal` that would have also caught this issue if someone were able to hit the edge case.
Fixes#22670
ACKs for top commit:
instagibbs:
utACK 92885c4f69
Tree-SHA512: bd61fa61ffb60873e097737eebea3afe8a42296ba429de9038b3a4706763b34de9409de6cdbab21ff7f51f4787b503f840873182d9c4a1d6e12a54b017953547
5fe8100ff3 Change the wallet_backup.py test to use the restorewallet RPC command instead of restoring wallets manually. (lsilva01)
ae23faba6f Add a new RPC command: restorewallet (lsilva01)
Pull request description:
As far as I know, there is no command to restore the wallet from a backup file.
The only way to do this is to replace the `wallet.dat` of a newly created wallet with the backup file, which is hardly an intuitive way.
This PR implements the `restorewallet` RPC command which restores the wallet from the backup file.
To test:
First create a backup file:
`$ bitcoin-cli -rpcwallet="wallet-01" backupwallet /home/Backups/wallet-01.bak`
Then restore it in another wallet:
`$ bitcoin-cli restorewallet "restored-wallet-01" /home/Backups/wallet-01.bak`
ACKs for top commit:
achow101:
re-ACK 5fe8100ff3
prayank23:
tACK 5fe8100ff3
meshcollider:
utACK 5fe8100ff3
Tree-SHA512: 9639df4d8ad32f255f5b868320dc69878bd9aceb3b471b49dfad500b67681e2d354292b5410982fbf18e25a44ed0c06fd4a0dd010e82807c2e00ff32e84047a1
The connection of the WalletFrame::currentWalletSet signal to
the BitcoinGUI::updateWalletStatus is a shorter and more descriptive way
to call both the setHDStatus and setEncryptionStatus member functions of
the BitcoinGUI class in comparison to using of the
WalletView::updateEncryptionStatus slot.
4844b74ba7 p2p: log addrman consistency checks (Jon Atack)
Pull request description:
This mini-patch picks up #22479 to log addrman consistency checks in the `BCLOG::ADDRMAN` category when they are enabled with the `-checkaddrman=<n>` configuration option for values of n greater than 0.
```
$ ./src/bitcoind -signet -checkaddrman=20 -debug=addrman
...
2021-08-13T11:14:45Z Addrman checks started: new 3352, tried 89, total 3441
2021-08-13T11:14:45Z Addrman checks completed successfully
```
This allows people to
- verify the checks are running
- see when and how often they are being performed
- see the number of new/tried/total addrman entries per check
- see the start/end of the checks
Thanks to John Newbery for ideas to improve this logging.
ACKs for top commit:
jnewbery:
Code review ACK 4844b74ba7
Zero-1729:
tACK 4844b74ba7
theStack:
Concept and code-review ACK 4844b74ba7♟️
Tree-SHA512: 10b51c480d52a753ea8a59dbdd1e2c4f49067e7f4afe59d58426a8fb438f52447fe3a6090fa52132bc382d876927fa338b229c906d85668086f7f8f5bd8ed38a
d930c7f5b0 p2p, rpc, test: address rate-limiting follow-ups (Jon Atack)
Pull request description:
Incorporates review feedback in #22387.
Edit, could be considered separately: should a release note (or two) be added for 22.0? e.g. the new getpeerinfo fields in `Updated RPCs` and the rate-limiting itself in `P2P and network changes`.
ACKs for top commit:
MarcoFalke:
review ACK d930c7f5b0
theStack:
re-ACK d930c7f5b0🌮
Zero-1729:
crACK d930c7f
Tree-SHA512: b2101cad87f59c238603f38bd8e8df7a4d48929794e4de9e0e0ff2afa935a68475c2d369aa669d124a0bec2f50280fb47e8b980bde6ad812db08cf67b71c066a
a4d78546b0 [addrman] Make addrman consistency checks a runtime option (John Newbery)
10aac24145 [tests] Make deterministic addrman use nKey = 1 (John Newbery)
fa9710f62c [addrman] Add deterministic argument to CAddrMan ctor (John Newbery)
ee458d84fc Add missing const to CAddrMan::Check_() (MarcoFalke)
Pull request description:
CAddrMan has internal consistency checks. Currently, these are only run when the program is compiled with the `DEBUG_ADDRMAN` option. This option is not enabled on any of our CI builds, and it's likely that no-one is running them at all.
This PR makes consistency checks a (hidden) runtime option that can be enabled with `-checkaddrman`, where `-checkaddrman=n` will result in the consistency checks running every n operations (similar to `-checkmempool=n`). We set the ratio to 1/100 for our unit tests, and leave it disabled by default for all networks. Additionally, a consistency check failure now asserts, rather than logging and continuing. This matches the behavior of CTxMemPool and TxRequestTracker, where a failed consistency check asserts.
ACKs for top commit:
jonatack:
ACK a4d78546b0 per `git diff 00fd089 a4d7854`, tested by adding logging similar to #22479 and running with `-checkaddrman=<n>` for various values 0/1/10/100 etc, tested the updated docs with `bitcoind -help-debug | grep -A2 "checkaddrman\|checkmempool"` and verified rebased on master that compiling with `CPPFLAGS="-DDEBUG_ADDRMAN"` no longer causes the build to error.
mzumsande:
Code-review ACK a4d78546b0
theStack:
Code-review ACK a4d78546b0
Tree-SHA512: eaee003f7a99154822c5b5efbc62008d32c1efbecc6fec6e183427f6b2ae5d30b3be7924e3a7271b1a1de91517f5bd2a70011d45358c3105c6a0702f12b70f7c
When the fee is not subtracted from the outputs, the amount that has
been reserved for the fee (change_and_fee - change_amount) must be
enough to cover the fee that is needed. It would be a bug to not do so,
so use an assert to make this obvious if such a situation were to occur.
For target value calculations, GetSelectionAmount should be used, not
m_effective_value or m_value.
Specifically, ApproximateBestSubset mistakenly uses m_value when
calculating whether the target value has been met. This has been changed
to use GetSelectionAmount.
93cc53a2b2 gui: Unregister wallet notifications before unloading wallets (Russell Yanofsky)
Pull request description:
This change was originally part of both bitcoin/bitcoin#10102 and bitcoin/bitcoin#19101 and is required for both because it avoids the IPC wallet implementation in bitcoin/bitcoin#10102 and the WalletContext implementation in bitcoin/bitcoin#19101 needing to deal with notification objects that have stale pointers to deleted wallets.
ACKs for top commit:
promag:
Code review ACK 93cc53a2b2.
hebasto:
ACK 93cc53a2b2
Tree-SHA512: 805f50a493291ad0f7c48725fbc5058d58ebbdb0770befd51d8aa241209a13f8a46f5982481336ab8338cdc83e9017668089a71deccf1587308e841cf8697825
Currently addrman consistency checks are a compile time option, and are not
enabled in our CI. It's unlikely anyone is running these consistency checks.
Make them a runtime option instead, where users can enable addrman
consistency checks every n operations (similar to mempool tests). Update
the addrman unit tests to do internal consistency checks every 100
operations (checking on every operations causes the test runtime to
increase by several seconds).
Also assert on a failed addrman consistency check to terminate program
execution.
62b125fd19 qt, refactor: Fix indentation (Prateek Sancheti)
ad28b66e98 qt: Add SubFeeFromAmount option (Prateek Sancheti)
Pull request description:
This PR adds **_SubFeeFromAmount_** option which lets the user select their preferred setting of whether fee for a transaction is to be subtracted from the amount or not for future transactions. The setting chosen by the user is remembered even when the GUI mode is turned off.
**_Functionality and Usage:_**
- Go to `Settings > Options > Wallet` on _Windows/Linux_ or `bitcoin-qt > Preferences > Wallet` on _macOS_.
- The checkbox **Subtract Fee From Amount** corresponds to the added option **SubFeeFromAmount**.
- The preferred setting intended to be the default for all future send transactions should be selected by the user.
- Click on **OK**.
- Go to the **Send** tab in the wallet.
- You shall notice, any new Send transaction created will have the preferred setting as chosen by the user.<br> (Try clicking on Add recipient or even restarting the Node in GUI)
Attaching ScreenRecordings to explain the added feature.
> Master.mov: Master Branch
https://user-images.githubusercontent.com/54016434/127763378-be91837d-d0ab-4ae5-87c0-d303fa70a336.mov
> PR.mov: PullRequest
https://user-images.githubusercontent.com/54016434/127763404-05b834c1-4082-4fbd-9b05-1528ac898a21.movClose#386
ACKs for top commit:
Talkless:
tACK 62b125fd19, tested on Debian Sid with 5.15.2 and it works as described.
hebasto:
re-ACK 62b125fd19, only removed the unused `SubFeeFromAmountChanged` signal since my [previous](https://github.com/bitcoin-core/gui/pull/390#pullrequestreview-726531766) review.
meshcollider:
utACK 62b125fd19
Tree-SHA512: 932ca89ae578a1e1c426561400d87cf005c231944feaf0f662ff8d88f32bdd65a927a090ea41510a15f8ec0ebcd5529672e9917720eb5ea85f413f081e45d5bb
Previously, building from a release source tarball would result in a
version string like v22.0.0-<commithash>, but we expect just v22.0.0.
This commit solves this problem.
Also use PACKAGE_VERSION instead of reconstructing it.
0237d95323 qt: Add Load PSBT functionaliy with nowallet (Prateek Sancheti)
Pull request description:
This PR provides a fix to the issue mentioned in #232.
Currently, the **_Load PSBT_** functionality works well in case a wallet is loaded but does nothing when a wallet isn't loaded.
If a function cannot work without a wallet being loaded, it is disabled by default (It is unclickable as shown in the image).
For e.g. One cannot `Close Wallet` or `Backup Wallet` or `Sign Messages` without a wallet being loaded. And hence they are disabled. But if you notice, `Load PSBT` options are not disabled by default even when a wallet isn't loaded.
> 
As mentioned by hebasto in the issue description :
```
<hebasto> achow101: does "File" -> "Load PSBT from {file|clipboard}" make any sense when no wallet is loaded?
<achow101> hebasto: yes, for finalize and sending
```
This means **_Load PSBT_** should be working just as similar whether wallets are being loaded or not.
After making the required changes to the code, The **_Load PSBT_** works as expected even with no wallet loaded and the PSBT is finalized.
| Master | PR |
|-------------|---------------|
|  |  |
Close#232
ACKs for top commit:
achow101:
re-ACK 0237d95323
hebasto:
ACK 0237d95323, tested on Linux Mint 20.2 (Qt 5.12.8).
Tree-SHA512: 8d928c5bfd3c2b286ddcacd0b367c872de8bc3d3d9d82280faeadc60d738b86af328c060b5763ade364c9b386b23f95580c2eb1147b16373fbb713170c100350
6971e790c3 gui: add Direction column to peers tab (Jon Atack)
Pull request description:
Picking up #289
This adds a `Direction column`, making the peers tab the same as the `Direction/Type` row in the peer details and the direction and type columns in our other user-facing peer connections table in `-netinfo`.
Users can now sort the peers table by direction. The default sort is set to inbound, then outbound.
| Master | PR |
| ----------- | ----------- |
|  |  |
ACKs for top commit:
jonatack:
Tested ACK 6971e790c3
ShaMan239:
tACK 6971e790c3
hebasto:
ACK 6971e790c3, tested on Linux Mint 20.2 (Qt 5.12.8).
Tree-SHA512: 9716cdedd435f88245a097fed6d4b2b486104d0dd09df739bdb4f2bfad709cbd9c9a231168cc3326e94fa5fddc77dd68f992f20417d04d94930db9fccdbb7de1
4830f4912a qt: Refactor open date range to use std::optional (João Barbosa)
Pull request description:
Use `std::nullopt` for open date range instead of `TransactionFilterProxy::MIN_DATE` and `TransactionFilterProxy::MAX_DATE`.
ACKs for top commit:
hebasto:
re-ACK 4830f4912a, only missed header included since my [previous](https://github.com/bitcoin-core/gui/pull/354#pullrequestreview-682108182) review.
Talkless:
tACK 4830f4912a, tested on Debian Sid, filtering seems to work as expected.
Tree-SHA512: dcecbcc129cb401d6ac13a20f015b8cb2a7434fae6bd3e5b19fca5531e8bd915e2a0835f9c601371381750cdc8cd6fcf4f8c6669177d679773046cbe13bed68b
bb56486a17 refactor: Reuse MakeUnorderedList where possible (Hennadii Stepanov)
77a90f03ac refactor: Move MakeUnorderedList into util/string.h to make it reusable (Hennadii Stepanov)
6a5ccd65c7 scripted-diff: Rename JoinErrors in more general MakeUnorderedList (Hennadii Stepanov)
Pull request description:
A nice `JoinErrors` utility function was introduced in https://github.com/bitcoin-core/gui/pull/379 by Russell Yanofsky.
This PR renames this function and re-uses it across the code base.
ACKs for top commit:
Zero-1729:
Concept ACK bb56486a17
theStack:
Code-review ACK bb56486a17
Talkless:
utACK bb56486a17
ryanofsky:
Code review ACK bb56486a17. Nice deduping, thanks for this!
Tree-SHA512: 6bdbfa61f2ffa69e075f46b733f247c6d5b8486779a1dac064285a199a4bb8bc5ef44eaee37086305646b5c88eb6a11990883219a4a9140a5117ee21ed529bb9
fafe896a0b test: Set regtest.BIP66Height = 102 to speed up tests (MarcoFalke)
Pull request description:
No need to waste time by forcing creation of more than 1000 blocks to get the benefits of being able to test BIP 66. Also, reducing the height makes it more likely that (third-party) tests are conforming to BIP 66, which is enforced on mainnet for all new blocks.
ACKs for top commit:
GeneFerneau:
Concept + code review ACK [fafe896](fafe896a0b)
0xB10C:
crACK fafe896a0b
laanwj:
ACK fafe896a0b
Zero-1729:
tACK fafe896
kristapsk:
ACK fafe896a0b. Full functional test suite showed few second speed incrase on my laptop (although I didn't do proper benchmarking with multiple runs, just single `time ./test/functional/test_runner.py` on current master vs this PR).
theStack:
Tested ACK fafe896a0b
hg333:
tACK fafe896a0b
Tree-SHA512: 4bbee3c8587d612e74a59fde49b6439c1296f2fc27d3a7cf59a35e920f729fdd581c930290bd04def618f81412236676ddb99b4ceb4d80dfb9fd610b128a04b1
b851a92c06 cli: Add progress bar for -getinfo (klementtan)
Pull request description:
Add a progress bar for the `Verification progress` attribute in `-getinfo` when verification progress `< 99%`.
**Motivation**:
* Improve user-friendliness of `-getinfo`
* Can be useful with `watch -n 1 bitcoin-cli -getinfo`(suggested by theStack [below](https://github.com/bitcoin/bitcoin/pull/22547#issuecomment-887488172))
* The progress bar is only display when are still syncing to tip(verification progress `< 99%`)
**Reviewing**
If your verification progress is `> 99%` you can restart the verification progress with
```shell
$ ./src/bitcoind -reindex
$./src/bitcoin-cli -getinfo
```
ACKs for top commit:
prayank23:
reACK b851a92c06
theStack:
re-ACK b851a92c06🍹
Zero-1729:
re-tACK b851a92c06 (re-tested, works as expected 🍾)
jonatack:
ACK b851a92c06
lsilva01:
Tested ACK b851a92c06 on mainnet and signet on Ubuntu 20.04.
Tree-SHA512: 2046d812e3c4623c6cc3ed4c24f2daaa92ba12cd181fa21626b782743890c2373be3175cff1441a7ba37295b6d5818368deea90d483959875c22f7ad9b601a20
accf3d5868 [test] mempool package ancestor/descendant limits (glozow)
2b6b26e57c [test] parameterizable fee for make_chain and create_child_with_parents (glozow)
313c09f7b7 [test] helper function to increase transaction weight (glozow)
f8253d69d6 extract/rename helper functions from rpc_packages.py (glozow)
3cd663a5d3 [policy] ancestor/descendant limits for packages (glozow)
c6e016aa13 [mempool] check ancestor/descendant limits for packages (glozow)
f551841d3e [refactor] pass size/count instead of entry to CalculateAncestorsAndCheckLimits (glozow)
97dd1c729d MOVEONLY: add helper function for calculating ancestors and checking limits (glozow)
f95bbf58aa misc package validation doc improvements (glozow)
Pull request description:
This PR implements a function to calculate mempool ancestors for a package and enforces ancestor/descendant limits on them as a whole. It reuses a portion of `CalculateMemPoolAncestors()`; there's also a small refactor to move the reused code into a generic helper function. Instead of calculating ancestors and descendants on every single transaction in the package and their ancestors, we use a "worst case" heuristic, treating every transaction in the package as each other's ancestor and descendant. This may overestimate everyone's counts, but is still pretty accurate in the our main package use cases, in which at least one of the transactions in the package is directly related to all the others (e.g. 1 parent + 1 child, multiple parents with 1 child, or chains).
Note on Terminology: While "package" is often used to describe groups of related transactions _within_ the mempool, here, I only use package to mean the group of not-in-mempool transactions we are currently validating.
#### Motivation
It would be a potential DoS vector to allow submission of packages to mempool without a proper guard for mempool ancestors/descendants. In general, the purpose of mempool ancestor/descendant limits is to limit the computational complexity of dealing with families during removals and additions. We want to be able to validate multiple transactions on top of the mempool, but also avoid these scenarios:
- We underestimate the ancestors/descendants during package validation and end up with extremely complex families in our mempool (potentially a DoS vector).
- We expend an unreasonable amount of resources calculating everyone's ancestors and descendants during package validation.
ACKs for top commit:
JeremyRubin:
utACK accf3d5
ariard:
ACK accf3d5.
Tree-SHA512: 0d18ce4b77398fe872e0b7c2cc66d3aac2135e561b64029584339e1f4de2a6a16ebab3dd5784f376e119cbafc4d50168b28d3bd95d0b3d01158714ade2e3624d
92993aa5cf Change SignTransaction's input_errors to use bilingual_str (Andrew Chow)
171366e89b Use bilingual_str for address fetching functions (Andrew Chow)
9571c69b51 Add bilingual_str::clear() (Andrew Chow)
Pull request description:
In a couple of places in the wallet, errors are `std::string`. In order for these errors to be translated, change them to use `bilingual_str`.
ACKs for top commit:
hebasto:
re-ACK 92993aa5cf, only rebased since my [previous](https://github.com/bitcoin/bitcoin/pull/22337#pullrequestreview-694542729) review, verified with
klementtan:
Code review ACK 92993aa5cf
meshcollider:
Code review ACK 92993aa5cf
Tree-SHA512: 5400e419dd87db8c49b67ed0964de2d44b58010a566ca246f2f0760ed9ef6a9b6f6df7a6adcb211b315b74c727bfe8c7d07eb5690b5922fa5828ceef4c83461f
fa6fd3dd6a wallet: Properly set fInMempool in mempool notifications (MarcoFalke)
Pull request description:
A wallet method (like bumping the fee) might have set `fInMempool` to false because the transaction was removed from the mempool (See commit fa4e088cba).
Avoid setting it back to true (incorrectly) in the validation interface background thread.
Fixes#22357
ACKs for top commit:
ryanofsky:
Code review ACK fa6fd3dd6a. Only change since last review is extending workaround to `transactionRemovedFromMempool`. Since we know this workaround is imperfect and the goal of this PR is mainly to fix CI errors, I would probably be inclined to limit the workaround to as few places as possible where we have seen actual failures, instead of adding the workaround to as many places as possible, where there is some chance it might trigger new failures. But since this workaround is so straightforward and almost looks like a real fix, probably it doesn't matter.
meshcollider:
utACK fa6fd3dd6a
Tree-SHA512: d690136a577f1f532aa1fee80d3f6600ff7fc61286fbf564a53d7938d5ae52d33f0dbb0fef8b8c041a4970fb424f0b9f1ee7ce791e0ff8354e0000ecc9e22b84
bb822a7af8 wallet, rpc: add listdescriptors private option (S3RK)
Pull request description:
Rationale: make it possible to backup your wallet with `listdescriptors` command
* The default behaviour is still to show public version
* For private version only the root xprv is returned
Example use-case:
```
> bitcoin-cli -regtest -named createwallet wallet_name=old descriptors=true
> bitcoin-cli -regtest -rpcwallet=old listdescriptors true | jq '.descriptors' > descriptors.txt
> bitcoin-cli -regtest -named createwallet wallet_name=new descriptors=true blank=true
> bitcoin-cli -regtest -rpcwallet=new importdescriptors "$(cat descriptors.txt)"
```
In case of watch-only wallet without private keys there will be following output:
```
error code: -4
error message:
Can't get descriptor string.
```
ACKs for top commit:
achow101:
re-ACK bb822a7af8
Rspigler:
tACK bb822a7af8
jonatack:
ACK bb822a7af8 per `git diff 2854ddc bb822a7`
prayank23:
tACK bb822a7af8
meshcollider:
Code review ACK bb822a7af8
Tree-SHA512: f6dddc72a74e5667071ccd77f8dce578382e8e29e7ed6a0834ac2e114a6d3918b59c2f194f4079b3259e13d9ba3b4f405619940c3ecb7a1a0344615aed47c43d
a9b9ca82da gui: ensure external signer option remains disabled without signers (Andrew Chow)
Pull request description:
When no external signers are available, the option to enable external signers should always be disabled. However the encrypt wallet checkbox can erroneously re-enable the external signer checkbox. To avoid this, CreateWalletDialog now stores whether signers were available during setSigners so that future calls to external_signer_checkbox->setEnabled can account for whether signers are available.
Fixes#395
ACKs for top commit:
hebasto:
ACK a9b9ca82da, tested on Linux Mint 20.2 (Qt 5.12.8).
Sjors:
tACK a9b9ca82da
jarolrod:
ACK a9b9ca82da
Tree-SHA512: 98951bcadc23fce99a66ea2d367c44360989e888c253845a767e1f7085c594562d0f099de4130f4a078c5072aa7806294097d976ee6407291f3d3c5a4a608b44
When no external signers are available, the option to enable external
signers should always be disabled. However the encrypt wallet checkbox
can erroneously re-enable the external signer checkbox. To avoid this,
CreateWalletDialog now stores whether signers were available during
setSigners so that future calls to external_signer_checkbox->setEnabled
can account for whether signers are available.
addrman_tests fail when consistency checks are enabled, since the tests
set the deterministic test addrman's nKey value to zero, which is an
invalid value. Change this so that deterministic addrman's nKey value is
set to 1.
This requires updating a few tests that are using magic values derived
from nKey being set to 0.
87651795d8 fuzz: check that ser+unser produces the same AddrMan (Vasil Dimov)
6408b24517 fuzz: move init code to the CAddrManDeterministic constructor (Vasil Dimov)
Pull request description:
Add a fuzz test that fills addrman with a pile of randomly generated addresses, serializes it to a stream, unserializes the stream to another addrman object and compares the two.
Some discussion of this already happened at https://github.com/jnewbery/bitcoin/pull/18.
ACKs for top commit:
practicalswift:
cr ACK 87651795d8
jonatack:
ACK 87651795d8 rebased to current master, reviewed, fuzz build, ran `FUZZ=addrman_serdeser src/test/fuzz/fuzz`
Tree-SHA512: 7eda79279f14f2649840bf752e575d7b02cbaad541f74f7254855ebd4a32da988f042d78aa9228983350283bb74dd0c71f51f04c0846889c3ba2f19f01a0c303
When calculating ancestor/descendant counts for transactions in the
package, as a heuristic, count every transaction in the package as an
ancestor and descendant of every other transaction in the package.
This may overestimate, but will not underestimate, the
ancestor/descendant counts. This shortcut still produces an accurate
count for packages of 1 parent + 1 child.
This does not change existing behavior.
The ancestor/descendant limits are inclusive of the entries themselves,
but CalculateAncestorsAndCheckLimits() does not need access to them.
faa670d386 test: Properly set BIP34 height in CreateNewBlock_validity unit test (MarcoFalke)
Pull request description:
The coinbase scriptSig in this unit test has several issues:
* The BIP34 height is not the "first item" as required (See https://github.com/bitcoin/bips/blob/master/bip-0034.mediawiki#specification)
* It uses the wrong encoding ( See da69d9965a/src/validation.cpp (L3250) )
* It uses the wrong height (off by one)
While BIP34 isn't currently enforced in this unit test, this should be fixed to avoid confusion and to promote self-consistency.
The change obviously requires new proof of work (`BLOCKINFO`).
Also change the block version from `1` to `VERSIONBITS_TOP_BITS`, because this test shouldn't care about the block version and bumping it is required for other changes.
ACKs for top commit:
theStack:
Code review ACK faa670d386
Tree-SHA512: 8dbe2d5300a640f3e1817ff048906e60463aca64ba50fec8ee4f18fb1c70e511008755b0b5baba81114a1a6265fdfae9a4b7ae8acadfb2c7ad43223157a0386c
6969b2bb98 qt, test: use regex search in apptests (Jarol Rodriguez)
d09d1cf1a2 qt, test: introduce FindInConsole function (Jarol Rodriguez)
Pull request description:
This PR refactors our GUI `apptests` so that it uses regex search to find values in our console/qtextedit output regardless if it is in `plaintext`, `html`, or `markdown`.
This introduces a new function `FindInConsole` which uses [QRegularExpression](https://doc.qt.io/qt-5/qregularexpression.html) to search the output of the console. The function must be provided with a [perl compatible regex](https://www.debuggex.com/cheatsheet/regex/pcre) pattern which wants to match a single group. The function then returns the matched group. If no match is found, an empty `QString` is returned.
We then use this new function in `TestRpcCommand` to find the current `chain` value instead of reading with univalue.
This approach can apply to a wider variety of testing scenarios as we can reuse this function to search for values when the console output is exported in a different format than `plaintext`. As an example, A follow up PR will add tests for console resizing and needs to look for the size in `html` tags after exporting the console text with `toHtml()`.
ACKs for top commit:
hebasto:
ACK 6969b2bb98
ShaMan239:
ACK 6969b2bb98
Tree-SHA512: 4db8bcd4a1acc4539ca64bbd7de572fe7dd6afc3e95108235abfc2891585bc4db3a56a33928fa38e8d44ac87023ce0dee3abcfadfbcd4440e3a21a52fef02536
Currently, this call to SetupAddressRelay will never return false because of
the previous guard that returns early if the peer is not an inbound connection.
Rather than implicitly relying on this guarantee, throw an error in the debug
build if it ever changes.
32fa49a184 make ParseOutputType return a std::optional<OutputType> (fanquake)
Pull request description:
Similar to #22220. Skipped using `auto` here for the same reasons outlined in that PR.
ACKs for top commit:
jnewbery:
utACK 32fa49a184
jonatack:
Code review ACK 32fa49a184 and debian clang 13 debug build is clean / unit tests locally are green
MarcoFalke:
review ACK 32fa49a184🍢
Tree-SHA512: 7752193117669b800889226185d49d164395697853828f8acb568f07651789bc5b2cddc45555957450353886e46b9a1e13c77a5e730a14c6ee621fabc8dc3d10
5e33f762d4 p2p, rpc: address relay fixups (Jon Atack)
Pull request description:
Following review of new changes merged today, move a use of `statestats` in getpeerinfo to within the section guarded by `if (fStateStats)`, e.g. `PeerManagerImpl::GetNodeStateStats` true, and pass an in-param by reference to const.
ACKs for top commit:
amitiuttarwar:
ACK 5e33f762d4
jnewbery:
ACK 5e33f762d4
Tree-SHA512: b42f33c615b14079e2c4e6060209de8707d71b351dd1e11e04a2a6fc12d15747d0c5d9b24850217080fd1ef92e63f96d6925c4badf280b781edd696c349be7d6
703b1e612a Close minor startup race between main and scheduler threads (Larry Ruane)
Pull request description:
This is a low-priority bug fix. The scheduler thread runs `CheckForStaleTipAndEvictPeers()` every 45 seconds (EXTRA_PEER_CHECK_INTERVAL). If its first run happens before the active chain is set up (`CChain::SetTip()`), `bitcoind` will assert:
```
(...)
2021-07-28T22:16:49Z init message: Loading block index…
bitcoind: validation.cpp:4968: CChainState& ChainstateManager::ActiveChainstate() const: Assertion `m_active_chainstate' failed.
Aborted (core dumped)
```
I ran into this while using the debugger to investigate an unrelated problem. Single-stepping through threads with a debugger can cause the relative thread execution timing to be very different than usual. I don't think any automated tests are needed for this PR. I'll give reproduction steps in the next PR comment.
ACKs for top commit:
MarcoFalke:
cr ACK 703b1e612a
tryphe:
tested ACK 703b1e612a
0xB10C:
ACK 703b1e612a
glozow:
code review ACK 703b1e612a - it makes sense to me to start peerman's background tasks here, after `chainstate->LoadChainTip()` and `node.connman->Start()` have been called.
Tree-SHA512: 9316ad768cba3b171f62e2eb400e3790af66c47d1886d7965edb38d9710fc8c8f8e4fb38232811c9346732ce311d39f740c5c2aaf5f6ca390ddc48c51a8d633b
036d7eadf5 doc: Correct description of CAddrMan::Create() (Amiti Uttarwar)
318176aff1 doc: Update high-level addrman description (Martin Zumsande)
Pull request description:
The high-level description of `addrman` has outdated information with respect to the eviction behavior, both for the New and Tried tables (at least since #5941) - this has confused me in the past.
This PR corrects this and also adds basic info about the bucket size and position.
ACKs for top commit:
amitiuttarwar:
reACK 036d7eadf5
jnewbery:
ACK 036d7eadf5
Tree-SHA512: 3f0635d765f5e580a1fae31187742a833cef66ef2286d40eeb28f2253521260038e16e5f1a65741464a2ddfdbeb5c0f1bc38bf73841e600639033d59c3c534e4
4a1b2a7ba7 [GetTransaction] remove unneeded `cs_main` lock acquire (Sebastian Falbesoner)
Pull request description:
This PR is a follow-up to #22383. For reading from the mempool, only `mempool.cs` needs to be locked (see [suggestion by MarcoFalke](https://github.com/bitcoin/bitcoin/pull/22383#discussion_r675069128)):
b620b2d58a/src/txmempool.h (L554-L558)
`CTxMemPool::get()` acquires this lock:
b620b2d58a/src/txmempool.cpp (L822-L829)
so we don't need to acquire any lock ourselves in `GetTransaction()`, as the other functions called in the remaining parts also don't need to have `cs_main` locked.
ACKs for top commit:
tryphe:
Concept ACK. tested 4a1b2a7ba7 but not extensively.
jnewbery:
Code review ACK 4a1b2a7ba7
Tree-SHA512: 60e869f72e65cf72cb144be1900ea7f3d87c12f322756994f6a3ed8cd975230b36c7c90c34b60bbf41f9186f4add36decaac1d4f0d0749fb5451b3938a8aa78c
059171009b consensus/params: simplify ValidDeployment check to avoid gcc warning (Anthony Towns)
Pull request description:
Simplifies the ValidDeployment check to only check the upperbound, relying on the lower bound to be trivially true due to enum values starting at the minimum value of the underlying type (which is checked at compile time in deploymentstatus.cpp). Avoids a "comparison always true" warning in some versions of gcc.
Fixes#22587
ACKs for top commit:
MarcoFalke:
review ACK 059171009b
tryphe:
retested ACK 059171009b
Tree-SHA512: e53b5d478b46d35ec476d004e3c92803afb874c138dd6ef3848861f4281cc113fe88921bc4ac74fd4decbf318ed776d3f816c3a1185f99dc36a5cfecfff51f7c
222290f543 test: Set BIP34Height = 2 for regtest (MarcoFalke)
fac90c55be test: Create all blocks with version 4 or higher (MarcoFalke)
Pull request description:
BIP34 is active on the current tip of mainnet, so all miners must obey it. It would be nice if it also was active in fresh regtest instances from the earliest time possible.
I changed the BIP34 height to `2`, so that the block at height=1 may be used to mine a duplicate coinbase. (Needed to test mainnet behaviour)
This pull is done in two commits:
* test: Create all blocks with version 4 or higher:
Now that BIP34 is activated earlier, we need to create blocks with a higher version number. Just bump it to 4 instead of 2 to avoid having to bump it again later.
* test: Set BIP34Height = 2 for regtest:
This fixes the BIP34 implementation in the tests (to match the one of the Core codebase) and updates the tests where needed
ACKs for top commit:
ajtowns:
ACK 222290f543
jonatack:
ACK 222290f543 tested and reviewed rebased to current master 5e213822f8
theStack:
Tested ACK 222290f543
Tree-SHA512: d69c637a62a64b8e87de8c7f0b305823d8f4d115c1852514b923625dbbcf9a4854b5bb3771ff41702ebf47c4c182a4442c6d7c0b9f282c95a34b83e56a73939b
65332b1178 [addrman] Remove RemoveInvalid() (John Newbery)
Pull request description:
PRs #22179 and #22112 (EDIT: later reverted in #22497) added hotfix code to addrman to remove invalid addresses and mutate the ports of I2P entries after entering into addrman. Those hotfixes included at least two addrman data corruption bugs:
- #22467 (Assertion `nRndPos1 < vRandom.size() && nRndPos2 < vRandom.size()' failed)
- #22470 (Changing I2P ports in addrman may wronly skip some entries from "new" buckets)
Hotfixing addrman is inherently dangerous. There are many members that have implicit assumptions on each others' state, and mutating those directly can lead to violating addrman's internal invariants.
Instead of trying to hotfix addrman, just don't insert any invalid addresses. For now, those are addresses which fail `CNetAddr::IsValid()`.
ACKs for top commit:
sipa:
utACK 65332b1178. I tried to reason through scenarios that could introduce inconsistencies with this code, but can't find any.
fanquake:
ACK 65332b1178 - Skipping the addition of invalid addresses (this code was initially added for Tor addrs) rather than adding all the invalids then removing them all when finishing unserializing seems like an improvement. Especially if it can be achieved with less code.
Tree-SHA512: 023113764cb475572f15da7bf9824b62b79e10a7e359af2eee59017df354348d2aeed88de0fd4ad7a9f89a0dad10827f99d70af6f1cb20abb0eca2714689c8d7
c020cbaa5c Squashed 'src/secp256k1/' changes from efad3506a8..be8d9c262f (Pieter Wuille)
Pull request description:
This updates our src/secp256k1 subtree to the lastest upstream master. Notable changes:
* New schnorrsig API (https://github.com/bitcoin-core/secp256k1/pull/844), which adds support for variable-length messages (not used in BIP341/342 transaction signing, so not relevant for us, but it changes the API, and makes some other simplifications). Some of our call sites had to be adapted.
* Don't use asm optimizations for `gen_context` (https://github.com/bitcoin-core/secp256k1/pull/965). This fixes#22441.
* Various testing/CI improvements
ACKs for top commit:
hebasto:
ACK e4ffb44716
jonatack:
Light ACK e4ffb44716 debug built (debian clang 13.0), ran bitcoind node/tests/git-subtree-check.sh, lightly reviewed the diff and API changes
fanquake:
ACK e4ffb44716
Tree-SHA512: 89a5c3019ec010d578e84bcef756d2c679420c5c768bcdece673405c4e10955179c5a1339aafc68b8b74b1e3912e147bf2f392f44f15af73791d93f6537960b3
82b6f89819 [style] Small style improvements to DNS parameters (Amiti Uttarwar)
4c89e24f64 [test] Test the delay before querying DNS seeds (Amiti Uttarwar)
6395c8ed56 [test] Test the interactions between -forcednsseed and -dnsseed (Amiti Uttarwar)
6f6b7df6bd [init] Disallow starting up with conflicting paramters for -dnsseed and -forcednsseed (Amiti Uttarwar)
26d0ffe4f2 [test] Test -forcednsseed causes querying DNS seeds (Amiti Uttarwar)
35851450a9 [test] Test the interactions between -connect and -dnsseed (Amiti Uttarwar)
75c05af361 [test] Test logic to query DNS seeds with block-relay-only connections (Amiti Uttarwar)
9c08719778 [test] Introduce test logic to query DNS seeds (Amiti Uttarwar)
Pull request description:
This PR adds a DNS seed to the regtest chain params to enable testing the DNS seed querying logic of `CConnman::ThreadDNSAddressSeed` and relevant startup parameters. Adds coverage for the changes in #22013 (and then some).
The main behavioral change to bitcoind is that this PR disallows starting up with conflicting parameters for `-dnsseed` and `-forcednsseed`.
The tests include:
* parameter interactions of different combinations of `-connect`, `-dnsseed` and `-forcednsseed`
* the delay before querying DNS seeds depending on how many addresses are in the addrman
* the behavior of `-forcednsseed`
* skipping DNS querying if we have outbound full relay connections & not block-relay-only connections
Huge props to mzumsande for identifying the timing technique for testing successful connections before running `ThreadDNSAddressSeed` 🙌🏽
ACKs for top commit:
mzumsande:
ACK 82b6f89819
jnewbery:
reACK 82b6f89819
Tree-SHA512: 9f0c29bfbf99426727e79c0a25606ae09deab91a92e3c5cee7f84c3ca7503a8ac9ab85a85c51841d40b164ef8c991326070f0b2f41d075fb7985df26f6e95d6d
3f7250b328 [test] Use the new endpoint to improve tests (Amiti Uttarwar)
3893da06db [RPC] Add field to getpeerinfo to indicate if addr relay is enabled (Amiti Uttarwar)
0980ca78cd [test] Test that we intentionally select addr relay peers. (Amiti Uttarwar)
c061599e40 [net_processing] Remove RelayAddrsWithPeer function (Amiti Uttarwar)
201e496481 [net_processing] Introduce new field to indicate if addr relay is enabled (Amiti Uttarwar)
1d1ef2db7e [net_processing] Defer initializing m_addr_known (Amiti Uttarwar)
6653fa3328 [test] Update p2p_addr_relay test to prepare (Amiti Uttarwar)
2fcaec7bbb [net_processing] Introduce SetupAddressRelay (Amiti Uttarwar)
Pull request description:
This PR builds on the test refactors extracted into #22306 (first 5 commits).
This PR aims to reduce addr blackholes. When we receive an `addr` message that contains 10 or less addresses, we forward them to 1-2 peers. This is the main technique we use for self advertisements, so sending to peers that wouldn't relay would effectively "blackhole" the trickle. Although we cannot prevent this in a malicious case, we can improve it for the normal, honest cases, and reduce the overall likelihood of occurrence. Two known cases where peers would not participate in addr relay are if they have connected to you as a block-relay-only connection, or if they are a light client.
This implementation defers initialization of `m_addr_known` until it is needed, then uses its presence to decide if the peer is participating in addr relay. For outbound (not block-relay-only) peers, we initialize the filter before sending the initial self announcement when processing their `version` message. For inbound peers, we initialize the filter if/when we get an addr related message (`ADDR`, `ADDRV2`, `GETADDR`). We do NOT initialize the filter based on a `SENDADDRV2` message.
To communicate about these changes beyond bitcoin core & to (try to) ensure that no other software would be disrupted, I have:
- Posted to the [mailing list](https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2021-April/018784.html)
- Researched other open source clients to confirm compatibility, opened issues in all the projects & documented in https://github.com/bitcoin/bitcoin/pull/21528#issuecomment-809906430. Many have confirmed that this change would not be problematic.
- Raised as topic during [bitcoin-core-dev meeting](https://www.erisian.com.au/bitcoin-core-dev/log-2021-03-25.html#l-954)
- Raised as topic during [bitcoin p2p meeting](https://www.erisian.com.au/bitcoin-core-dev/log-2021-04-20.html#l-439)
ACKs for top commit:
jnewbery:
reACK 3f7250b328
glozow:
ACK 3f7250b328
ajtowns:
utACK 3f7250b328
Tree-SHA512: 29069282af684c1cd37d107c395fdd432dcccb11626f3c2dabfe92fdc4c85e74c7c4056fbdfa88017fec240506639b72ac6c311f8ce7c583112eb15f47e421af
fa1eddb1a3 Fix whitespace in touched files (MarcoFalke)
fa4e6afdae Remove unused CSubNet serialize code (MarcoFalke)
fa384fdd0b Ignore banlist.dat (MarcoFalke)
Pull request description:
The code to read `banlist.dat` should be removed eventually. The major release (22.x) can be used to translate a `banlist.dat` into a `banlist.json`. Thus, it is now possible to remove the reading code.
ACKs for top commit:
Zero-1729:
re-ACK fa1eddb1a3
laanwj:
concept and code review ACK fa1eddb1a3
vasild:
ACK fa1eddb1a3
jonatack:
Light code review utACK fa1eddb1a3
Tree-SHA512: e136193b7c0ba1d6d2e79c7fb4106ba4af75fa229ed7214675ee64e98e59bb4808779e7a8a09eecce62f7a5d4bc6e16b8a5ad4596129357c8fc5e3b88f214249
fae108ceb5 Fix incorrect whitespace in addrman (MarcoFalke)
fa32024d51 Add missing GUARDED_BY to CAddrMan::insecure_rand (MarcoFalke)
fab755b77f fuzz: Actually use const addrman (MarcoFalke)
fae0c79351 refactor: Mark CAddrMan::GetAddr const (MarcoFalke)
fa02934c8c refactor: Mark CAddrMan::Select const (MarcoFalke)
Pull request description:
To clarify that a call to this only changes the random state and nothing else.
ACKs for top commit:
jnewbery:
Code review ACK fae108ceb5
theStack:
re-ACK fae108ceb5🍦
Tree-SHA512: 3ffb211d4715cc3daeb3bfcdb3fcc6b108ca96df5fa565510436fac0e8da86c93b30c9c4aad0563e27d84f615fcd729481072009a4e2360c8b3d40787ab6506a
Don't schedule class PeerManagerImpl's background tasks from its
constructor, but instead do that from a separate method,
StartScheduledTasks(), that can be called later at the end of startup,
after other things, such as the active chain, are initialzed.
-dnsseed determines whether we run ThreadDNSAddressSeed and potentially query
the DNS seeds for addresses. -forcednsseed tells the node to force querying the
DNS seeds even if we have sufficient addresses or current connections.
This commit disallows starting up with explicitly conflicting parameters.
This commit introduces a DNS seed to the regest chain params in order to add
coverage to the DNS querying logic.
The first test checks that we do not query DNS seeds if we are able to
succesfully connect to 2 outbound connections. Since we participate in ADDR
relay with those connections, including sending a GETADDR message during the
VERSION handshake, querying the DNS seeds is unnecessary.
Co-authored-by: Martin Zumsande <mzumsande@gmail.com>
Leaving the incorrect indentation would be frustrating because:
* Some editor may fix up the whitespace when editing a file, so before
commiting the whitespace changes need to be undone.
* It makes it harder to use clang-format-diff on a change.
Can be trivially reviewed with --word-diff-regex=. --ignore-all-space
Use SetupAddressRelay to only initialize `m_addr_known` as needed. For outbound
peers, we initialize the filter before sending our self announcement (not
applicable for block-relay-only connections). For inbound peers, we initialize
the filter when we get an addr related message (ADDR, ADDRV2, GETADDR).
These changes intend to mitigate address blackholes. Since an inbound peer has
to send us an addr related message to become eligible as a candidate for addr
relay, this should reduce our likelihood of sending them self-announcements.
Idempotent function that initializes m_addr_known for connections that support
address relay (anything other than block-relay-only). Unused until the next
commit.
e4c8bb62e4 build: Fix undefined reference to __mulodi4 (Hennadii Stepanov)
Pull request description:
When compiling with clang on 32-bit systems the `__mulodi4` symbol is defined in compiler-rt only.
Fixes#21294.
See more:
- https://bugs.llvm.org/show_bug.cgi?id=16404
- https://bugs.llvm.org/show_bug.cgi?id=28629
ACKs for top commit:
MarcoFalke:
tested-only ACK e4c8bb62e4
luke-jr:
utACK e4c8bb62e4
fanquake:
ACK e4c8bb62e4 - it's a bit of an awkward workaround to carry, but at-least it's contained to the fuzzers.
Tree-SHA512: 93edb4ed568027702b1b9aba953ad50889b834ef97fde3cb99d1ce70076d9c00aa13f95c86b12d6f59b24fa90108d93742f920e15119901a2848fb337ab859a1
f685a13bef doc: GetTransaction()/getrawtransaction follow-ups to #22383 (John Newbery)
abc57e1f08 refactor: move `GetTransaction(...)` to node/transaction.cpp (Sebastian Falbesoner)
Pull request description:
~This PR is based on #22383, which should be reviewed first~ (merged by now).
In [yesterday's PR review club session to PR 22383](https://bitcoincore.reviews/22383), the idea of moving the function `GetTransaction(...)` from src/validation.cpp to src/node/transaction.cpp came up. With this, the circular dependency "index/txindex -> validation -> index/txindex" is removed (see change in `lint-circular-dependencies.sh`). Thanks to jnewbery for suggesting and to sipa for providing historical background.
Relevant IRC log:
```
17:52 <jnewbery> Was anyone surprised that GetTransaction() is in validation.cpp? It seems to me that node/transaction.cpp would be a more appropriate place for it.
17:53 <raj_> jnewbery, +1
17:53 <stickies-v> agreed!
17:54 <glozow> jnewbery ya
17:54 <jnewbery> seems weird that validation would call into txindex. I wonder if we remove this function, then validation would no longer need to #include txindex
17:54 <sipa> GetTransaction predates node/transaction.cpp, and even the generic index framework itself :)
17:55 <sipa> (before 0.8, validation itself used the txindex)
17:55 <jnewbery> (and GetTransaction() seems like a natural sibling to BroadcastTransaction(), which is already in node/transaction.cpp)
17:55 <jnewbery> sipa: right, this is not meant as a criticism of course. Just wondering if we can organize things a bit more rationally now that we have better separation between things.
17:55 <sipa> jnewbery: sure, just providing background
17:56 <sipa> seems very reasonable to move it elsewhere now
```
The commit should be trivial to review with `--color-moved`.
ACKs for top commit:
jnewbery:
Code review ACK f685a13bef
rajarshimaitra:
tACK f685a13bef
mjdietzx:
crACK f685a13bef
LarryRuane:
Code review, test ACK f685a13bef
Tree-SHA512: 0e844a6ecb1be04c638b55bc4478c2949549a4fcae01c984eee078de74d176fb19d508fc09360a62ad130677bfa7daf703b67870800e55942838d7313246248c
779e638ca9 coinstats: Add comments for new coinstatsindex values (Fabian Jahr)
5b3d4e724f Index: Improve logging in coinstatsindex (Fabian Jahr)
d4356d4e48 rpc: Block until synced if coinstatsindex is used in gettxoutsetinfo (Fabian Jahr)
a5f6791139 rpc: Add missing gettxoutsetinfo help docs (Fabian Jahr)
01386bfd88 Index: Return early from failed coinstatsindex init (Fabian Jahr)
1e3842385b index: Use batch writing in coinstatsindex WriteBlock (Fabian Jahr)
fb65dde147 scripted-diff: Fix coinstats data member names (Fabian Jahr)
8ea8c927ac index: Avoid unnecessary type casts in coinstatsindex (Fabian Jahr)
Pull request description:
This is a collection of smaller follow-ups to #19521, addressing several post-merge review comments.
ACKs for top commit:
Sjors:
re-utACK 779e638ca9
jonatack:
re-ACK 779e638ca9 diff since last review involves doc changes only; rebased to current master and verified clean debug build/no silent conflicts, unit tests, and feature_coinstatsindex functional test
laanwj:
Code review ACK 779e638ca9
Talkless:
re-utACK 779e638ca9 after cosmetic changes.
Tree-SHA512: cb0d038d230c582d7fe3041c89b1e04d39971fab3739d540c609cf826754c6c513b12ded08ac92180aec7a9d7a70114ece50357bd1a902de4adaae9f30b8d699
8858e88c84 p2p: refactor: tidy up `PeerManagerImpl::Misbehaving(...)` (Sebastian Falbesoner)
Pull request description:
This simple refactoring PR has the goal to improve the readability of the `Misbehaving` method by
- introducing constant variables `score_before` and `score_now` (to avoid repeatedly calculating the former)
- deduplicating calls to LogPrint(), eliminates else-branch
ACKs for top commit:
jnewbery:
utACK 8858e88c84
rajarshimaitra:
tACK 8858e88c84
Tree-SHA512: 1d4dd5ac1d16ee9595edf4fa46e4960915a203641d74e6c33cffaba62ea71328834309a4451256fb45daf759f0cf6f4f199c46815afff6c89c0746e2ad4d4092
fe6dc76b7c wallet test: Add test for subtract fee from recipient behavior (Russell Yanofsky)
2565478c81 wallet test refactor: add CreateSyncedWallet function (Russell Yanofsky)
Pull request description:
This adds test coverage for wallet subtract from recipient behavior without changing it. Behavior seems to have changed recently in a minor way in #17331 without being noticed.
ACKs for top commit:
achow101:
ACK fe6dc76b7c
glozow:
ACK fe6dc76b7c
promag:
Code review ACK fe6dc76b7c.
Tree-SHA512: e00c5dfe467e4ccef5edb0dd4fff6c53f35a37828a4327bea2e166751e5ef971d519ffca7b8f735b12912bb4a547980626356bc1855981005aed1a6c2a57be0b
- introduce constant variables `score_before` and
`score_after` in order to improve readability
- deduplicate calls to LogPrint(), eliminates else-branch
During initial sync after startup the gettxoutsetinfo RPC will still return an error while catching up. However, after the initial sync the index will not error immediately anymore when it's in the process of syncing to the tip while being called. Instead it will block until synced and then return the response.
fa33ed4b3f fuzz: Limit max ops in tx_pool fuzz targets (MarcoFalke)
Pull request description:
Without a size limit on the input data, the runtime is unbounded. Fix this by picking an upper bound on the maximum number of fuzz operations.
Reproducer from OSS-Fuzz (without bug report):
[clusterfuzz-testcase-tx_pool_standard-5963992253202432.log](https://github.com/bitcoin/bitcoin/files/6822465/clusterfuzz-testcase-tx_pool_standard-5963992253202432.log)
ACKs for top commit:
practicalswift:
cr ACK fa33ed4b3f
Tree-SHA512: 32098d573880afba12d510ac83519dc886a6c65d5207edb810f92c7c61edf5e2fc9c57e7b7a1ae656c02ce14e3595707dd6b93caf7956beb2bc817609e14d23d
faa86b71ac fuzz: Use ConsumeUInt256 helper to simplify rolling_bloom_filter fuzz test (MarcoFalke)
aaaa61fd30 fuzz: Speed up rolling_bloom_filter fuzz test (MarcoFalke)
Pull request description:
Without a size limit on the input data, the runtime is unbounded. Fix this by picking an upper bound on the maximum number of fuzz operations.
Reproducer from OSS-Fuzz (without bug report):
[clusterfuzz-testcase-rolling_bloom_filter-5980807721254912.log](https://github.com/bitcoin/bitcoin/files/6822159/clusterfuzz-testcase-rolling_bloom_filter-5980807721254912.log)
ACKs for top commit:
practicalswift:
cr ACK faa86b71ac
theStack:
Concept and code review ACK faa86b71ac
Tree-SHA512: eace588509dfddb2ba97baf86379fa713fa6eb758184abff676cb95807ff8ff36905eeaddeba05665b8464c35c57e2138f88caec71cbfb255e546bbe76558da0
faafda232e fuzz: Speed up prevector fuzz target (MarcoFalke)
Pull request description:
Without a size limit on the input data, the runtime is unbounded. Fix this by picking an upper bound on the maximum number of fuzz operations.
Should fix https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=35981
ACKs for top commit:
practicalswift:
cr ACK faafda232e
Tree-SHA512: 1bf166c4a99a8ce88bdc030cd6a32ce1da5251b73873772e0e9c001ec2bacafebb183f7c8c88806d0ab633aada2cff8b78791f5c9c0c6f2cc8ef5f0875c4b2ef
fa8bed6a47 fuzz: Temporarily disable failing assert in banman fuzz test (MarcoFalke)
Pull request description:
Otherwise the remainder of the fuzz test can't be fuzzed without running into crashes
ACKs for top commit:
practicalswift:
cr ACK fa8bed6a47
Tree-SHA512: ec6606292e2cfd26484c7f6caf1c418c377da54111b332990fce68373f0438defda71d931a42ca34431527fbc172dd2fdf29b260afca15b34910ee137de1c365
c3e111a7da Reduced number of validations in `tx_validationcache_tests` to keep the run time reasonable. (lucash-dev)
Pull request description:
Following a suggestion in the comments, changed `ValidateCheckInputsForAllFlags` from testing all possible flag combinations to testing a random subset. Also created a new enum constant for the highest flag, so that this test doesn’t keep testing an incomplete subset in case a new flag is added.
Timing for `checkinputs_test`:
```
Before: 6.8s
After: 3.7s
----------------
Saved: 3.1s (45%)
```
This PR was split from #13050. Also see #10026.
ACKs for top commit:
leonardojobim:
tACK c3e111a7da.
kallewoof:
ACK c3e111a7da
theStack:
re-ACK c3e111a7da
Tree-SHA512: bef49645bdd4f61ec73cc77a9f028b95d9856db9446d2e7fc9a48867a6f0e94c2c9f150cb771a30fe852db0efb0a1bd15d38b00d712651793ccb59ff6157a7b4
78f4c8b98e prefer to use txindex if available for GetTransaction (Jameson Lopp)
Pull request description:
Fixes#22382
Motivation: prevent excessive disk reads if txindex is enabled.
Worth noting that this could be argued to be less of a bug and more of an issue of undefined behavior. If a user calls GetTransaction with the wrong block hash, what should happen?
ACKs for top commit:
jonatack:
ACK 78f4c8b98e
theStack:
Code review ACK 78f4c8b98e
LarryRuane:
tACK 78f4c8b98e
luke-jr:
utACK 78f4c8b98e
jnewbery:
utACK 78f4c8b98e
rajarshimaitra:
Code review ACK 78f4c8b98e
lsilva01:
Code Review ACK and Tested ACK 78f4c8b98e on Ubuntu 20.04
Tree-SHA512: af7db5b98cb2ae4897b28476b2fa243bf7e6f850750d9347062fe8013c5720986d1a3c808f80098e5289bd84b085de03c81a44e584dc28982f721c223651bfe0
a806647d26 [validation] Always include merkle root in coinbase commitment (Dhruv Mehta)
189128c220 [validation] Set witness script flag with p2sh for blocks (Dhruv Mehta)
ac82b99db7 [p2p] remove redundant NODE_WITNESS checks (Dhruv Mehta)
6f8b198b82 [p2p] remove unused segwitheight=-1 option (Dhruv Mehta)
eba5b1cd64 [test] remove or move tests using `-segwitheight=-1` (Dhruv Mehta)
Pull request description:
Builds on #21009 and makes progress on remaining items in #17862
Removing `RewindBlockIndex()` in #21009 allows the following:
- removal of tests using `segwitheight=-1` in `p2p_segwit.py`.
- move `test_upgrade_after_activation()` out of `p2p_segwit.py` reducing runtime
- in turn, that allows us to drop support for `-segwitheight=-1`, which is only supported for that test.
- that allows us to always set `NODE_WITNESS` in our local services. The only reason we don't do that is to support `-segwitheight=-1`.
- that in turn allows us to drop all of the `GetLocalServices() & NODE_WITNESS` checks inside `net_processing.cpp`, since our local services would always include `NODE_WITNESS`
ACKs for top commit:
mzumsande:
Code-Review ACK a806647d26
laanwj:
Code review ACK a806647d26, nice cleanup
jnewbery:
utACK a806647d26
theStack:
ACK a806647d26
Tree-SHA512: 73e1a69d1d7eca1f5c38558ec6672decd0b60b16c2ef6134df6f6af71bb159e6eea160f9bb5ab0eb6723c6632d29509811e29469d0d87abbe9b69a2890fbc73e
aaaa9c6019 fuzz: Extend addrman fuzz test with deserialize (MarcoFalke)
Pull request description:
Requested on IRC:
```
[18:01] <vasild> => I think there is a good chance fuzzing addrman unserialize will find more bugs
[18:04] <sipa> definitely
ACKs for top commit:
jonatack:
ACK aaaa9c6019 per `git diff fa74025 aaaa9c6`
vasild:
ACK aaaa9c6019
Tree-SHA512: f57d0aecf22a933e48d3181d7398218949588dd0de31218d1d28c825649e55fd60b0de6fbc92d2497cf5639a4adc2061c9bf8216546a2be916feac4f03f16e8f
7b3a20b260 mempool: apply rule of 5 to epochguard.h, fix compiler warnings (Jon Atack)
Pull request description:
Apply the rule of five to `src/util/epochguard.h::{Epoch, Marker}` for safety, which also nicely fixes the `-Wdeprecated-copy` compiler warnings with Clang 13.
References:
- https://en.cppreference.com/w/cpp/language/rule_of_three
- https://www.stroustrup.com/C++11FAQ.html#default
- https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rc-five
<details><summary>Compiler warnings fixed</summary><p>
```bash
In file included from policy/rbf.cpp:5:
In file included from ./policy/rbf.h:8:
In file included from ./txmempool.h:24:
./util/epochguard.h:53:17: warning: definition of implicit copy constructor for 'Marker' is deprecated because it has a user-declared copy assignment operator [-Wdeprecated-copy]
Marker& operator=(const Marker&) = delete;
^
./txmempool.h:81:7: note: in implicit copy constructor for 'Epoch::Marker' first required here
class CTxMemPoolEntry
^
policy/rbf.cpp:29:29: note: in implicit copy constructor for 'CTxMemPoolEntry' first required here
CTxMemPoolEntry entry = *pool.mapTx.find(tx.GetHash());
```
```bash
In file included from txmempool.cpp:6:
In file included from ./txmempool.h:24:
./util/epochguard.h:53:17: warning: definition of implicit copy constructor for 'Marker' is deprecated because it has a user-declared copy assignment operator [-Wdeprecated-copy]
Marker& operator=(const Marker&) = delete;
^
./txmempool.h:81:7: note: in implicit copy constructor for 'Epoch::Marker' first required here
class CTxMemPoolEntry
^
/usr/bin/../lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/ext/new_allocator.h:150:23: note: in implicit copy constructor for 'CTxMemPoolEntry' first required here
{ ::new((void *)__p) _Up(std::forward<_Args>(__args)...); }
^
/usr/bin/../lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/bits/alloc_traits.h:512:8: note: in instantiation of function template specialization '__gnu_cxx::new_allocator<boost::multi_index::detail::hashed_index_node<boost::multi_index::detail::hashed_index_node<boost::multi_index::detail::ordered_index_node<boost::multi_index::detail::null_augment_policy, boost::multi_index::detail::ordered_index_node<boost::multi_index::detail::null_augment_policy, boost::multi_index::detail::ordered_index_node<boost::multi_index::detail::null_augment_policy, boost::multi_index::detail::index_node_base<CTxMemPoolEntry, std::allocator<CTxMemPoolEntry>>>>>>>>::construct<CTxMemPoolEntry, const CTxMemPoolEntry &>' requested here
__a.construct(__p, std::forward<_Args>(__args)...);
^
/usr/include/boost/multi_index_container.hpp:655:24: note: in instantiation of function template specialization 'std::allocator_traits<std::allocator<boost::multi_index::detail::hashed_index_node<boost::multi_index::detail::hashed_index_node<boost::multi_index::detail::ordered_index_node<boost::multi_index::detail::null_augment_policy, boost::multi_index::detail::ordered_index_node<boost::multi_index::detail::null_augment_policy, boost::multi_index::detail::ordered_index_node<boost::multi_index::detail::null_augment_policy, boost::multi_index::detail::index_node_base<CTxMemPoolEntry, std::allocator<CTxMemPoolEntry>>>>>>>>>::construct<CTxMemPoolEntry, const CTxMemPoolEntry &>' requested here
node_alloc_traits::construct(
^
/usr/include/boost/multi_index/detail/index_base.hpp:108:15: note: in instantiation of member function 'boost::multi_index::multi_index_container<CTxMemPoolEntry, boost::multi_index::indexed_by<boost::multi_index::hashed_unique<mempoolentry_txid, SaltedTxidHasher>, boost::multi_index::hashed_unique<boost::multi_index::tag<index_by_wtxid>, mempoolentry_wtxid, SaltedTxidHasher>, boost::multi_index::ordered_non_unique<boost::multi_index::tag<descendant_score>, boost::multi_index::identity<CTxMemPoolEntry>, CompareTxMemPoolEntryByDescendantScore>, boost::multi_index::ordered_non_unique<boost::multi_index::tag<entry_time>, boost::multi_index::identity<CTxMemPoolEntry>, CompareTxMemPoolEntryByEntryTime>, boost::multi_index::ordered_non_unique<boost::multi_index::tag<ancestor_score>, boost::multi_index::identity<CTxMemPoolEntry>, CompareTxMemPoolEntryByAncestorFee>>>::construct_value' requested here
final().construct_value(x,v);
^
/usr/include/boost/multi_index/detail/ord_index_impl.hpp:770:33: note: (skipping 5 contexts in backtrace; use -ftemplate-backtrace-limit=0 to see all)
final_node_type* res=super::insert_(v,x,variant);
^
/usr/include/boost/multi_index_container.hpp:693:33: note: in instantiation of function template specialization 'boost::multi_index::detail::hashed_index<mempoolentry_txid, SaltedTxidHasher, std::equal_to<uint256>, boost::multi_index::detail::nth_layer<1, CTxMemPoolEntry, boost::multi_index::indexed_by<boost::multi_index::hashed_unique<mempoolentry_txid, SaltedTxidHasher>, boost::multi_index::hashed_unique<boost::multi_index::tag<index_by_wtxid>, mempoolentry_wtxid, SaltedTxidHasher>, boost::multi_index::ordered_non_unique<boost::multi_index::tag<descendant_score>, boost::multi_index::identity<CTxMemPoolEntry>, CompareTxMemPoolEntryByDescendantScore>, boost::multi_index::ordered_non_unique<boost::multi_index::tag<entry_time>, boost::multi_index::identity<CTxMemPoolEntry>, CompareTxMemPoolEntryByEntryTime>, boost::multi_index::ordered_non_unique<boost::multi_index::tag<ancestor_score>, boost::multi_index::identity<CTxMemPoolEntry>, CompareTxMemPoolEntryByAncestorFee>>, std::allocator<CTxMemPoolEntry>>, boost::mpl::vector0<>, boost::multi_index::detail::hashed_unique_tag>::insert_<boost::multi_index::detail::lvalue_tag>' requested here
final_node_type* res=super::insert_(v,x,variant);
^
/usr/include/boost/multi_index_container.hpp:705:12: note: in instantiation of function template specialization 'boost::multi_index::multi_index_container<CTxMemPoolEntry, boost::multi_index::indexed_by<boost::multi_index::hashed_unique<mempoolentry_txid, SaltedTxidHasher>, boost::multi_index::hashed_unique<boost::multi_index::tag<index_by_wtxid>, mempoolentry_wtxid, SaltedTxidHasher>, boost::multi_index::ordered_non_unique<boost::multi_index::tag<descendant_score>, boost::multi_index::identity<CTxMemPoolEntry>, CompareTxMemPoolEntryByDescendantScore>, boost::multi_index::ordered_non_unique<boost::multi_index::tag<entry_time>, boost::multi_index::identity<CTxMemPoolEntry>, CompareTxMemPoolEntryByEntryTime>, boost::multi_index::ordered_non_unique<boost::multi_index::tag<ancestor_score>, boost::multi_index::identity<CTxMemPoolEntry>, CompareTxMemPoolEntryByAncestorFee>>>::insert_<boost::multi_index::detail::lvalue_tag>' requested here
return insert_(v,detail::lvalue_tag());
^
/usr/include/boost/multi_index/detail/index_base.hpp:213:21: note: in instantiation of member function 'boost::multi_index::multi_index_container<CTxMemPoolEntry, boost::multi_index::indexed_by<boost::multi_index::hashed_unique<mempoolentry_txid, SaltedTxidHasher>, boost::multi_index::hashed_unique<boost::multi_index::tag<index_by_wtxid>, mempoolentry_wtxid, SaltedTxidHasher>, boost::multi_index::ordered_non_unique<boost::multi_index::tag<descendant_score>, boost::multi_index::identity<CTxMemPoolEntry>, CompareTxMemPoolEntryByDescendantScore>, boost::multi_index::ordered_non_unique<boost::multi_index::tag<entry_time>, boost::multi_index::identity<CTxMemPoolEntry>, CompareTxMemPoolEntryByEntryTime>, boost::multi_index::ordered_non_unique<boost::multi_index::tag<ancestor_score>, boost::multi_index::identity<CTxMemPoolEntry>, CompareTxMemPoolEntryByAncestorFee>>>::insert_' requested here
{return final().insert_(x);}
^
/usr/include/boost/multi_index/hashed_index.hpp:284:46: note: in instantiation of member function 'boost::multi_index::detail::index_base<CTxMemPoolEntry, boost::multi_index::indexed_by<boost::multi_index::hashed_unique<mempoolentry_txid, SaltedTxidHasher>, boost::multi_index::hashed_unique<boost::multi_index::tag<index_by_wtxid>, mempoolentry_wtxid, SaltedTxidHasher>, boost::multi_index::ordered_non_unique<boost::multi_index::tag<descendant_score>, boost::multi_index::identity<CTxMemPoolEntry>, CompareTxMemPoolEntryByDescendantScore>, boost::multi_index::ordered_non_unique<boost::multi_index::tag<entry_time>, boost::multi_index::identity<CTxMemPoolEntry>, CompareTxMemPoolEntryByEntryTime>, boost::multi_index::ordered_non_unique<boost::multi_index::tag<ancestor_score>, boost::multi_index::identity<CTxMemPoolEntry>, CompareTxMemPoolEntryByAncestorFee>>, std::allocator<CTxMemPoolEntry>>::final_insert_' requested here
std::pair<final_node_type*,bool> p=this->final_insert_(x);
^
txmempool.cpp:363:53: note: in instantiation of member function 'boost::multi_index::detail::hashed_index<mempoolentry_txid, SaltedTxidHasher, std::equal_to<uint256>, boost::multi_index::detail::nth_layer<1, CTxMemPoolEntry, boost::multi_index::indexed_by<boost::multi_index::hashed_unique<mempoolentry_txid, SaltedTxidHasher>, boost::multi_index::hashed_unique<boost::multi_index::tag<index_by_wtxid>, mempoolentry_wtxid, SaltedTxidHasher>, boost::multi_index::ordered_non_unique<boost::multi_index::tag<descendant_score>, boost::multi_index::identity<CTxMemPoolEntry>, CompareTxMemPoolEntryByDescendantScore>, boost::multi_index::ordered_non_unique<boost::multi_index::tag<entry_time>, boost::multi_index::identity<CTxMemPoolEntry>, CompareTxMemPoolEntryByEntryTime>, boost::multi_index::ordered_non_unique<boost::multi_index::tag<ancestor_score>, boost::multi_index::identity<CTxMemPoolEntry>, CompareTxMemPoolEntryByAncestorFee>>, std::allocator<CTxMemPoolEntry>>, boost::mpl::vector0<>, boost::multi_index::detail::hashed_unique_tag>::insert' requested here
indexed_transaction_set::iterator newit = mapTx.insert(entry).first;
```
```bash
In file included from test/fuzz/policy_estimator.cpp:9:
In file included from ./test/fuzz/util.h:27:
In file included from ./txmempool.h:24:
./util/epochguard.h:53:17: warning: definition of implicit copy constructor for 'Marker' is deprecated because it has a user-declared copy assignment operator [-Wdeprecated-copy]
Marker& operator=(const Marker&) = delete;
^
./txmempool.h:81:7: note: in implicit copy constructor for 'Epoch::Marker' first required here
class CTxMemPoolEntry
^
/usr/bin/../lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/ext/new_allocator.h:150:23: note: in implicit move constructor for 'CTxMemPoolEntry' first required here
{ ::new((void *)__p) _Up(std::forward<_Args>(__args)...); }
^
/usr/bin/../lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/bits/alloc_traits.h:512:8: note: in instantiation of function template specialization '__gnu_cxx::new_allocator<CTxMemPoolEntry>::construct<CTxMemPoolEntry, CTxMemPoolEntry>' requested here
__a.construct(__p, std::forward<_Args>(__args)...);
^
/usr/bin/../lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/bits/vector.tcc:115:21: note: in instantiation of function template specialization 'std::allocator_traits<std::allocator<CTxMemPoolEntry>>::construct<CTxMemPoolEntry, CTxMemPoolEntry>' requested here
_Alloc_traits::construct(this->_M_impl, this->_M_impl._M_finish,
^
/usr/bin/../lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/bits/stl_vector.h:1204:9: note: in instantiation of function template specialization 'std::vector<CTxMemPoolEntry>::emplace_back<CTxMemPoolEntry>' requested here
{ emplace_back(std::move(__x)); }
^
test/fuzz/policy_estimator.cpp:49:37: note: in instantiation of member function 'std::vector<CTxMemPoolEntry>::push_back' requested here
mempool_entries.push_back(ConsumeTxMemPoolEntry(fuzzed_data_provider, tx));
```
</p></details>
ACKs for top commit:
laanwj:
Code review ACK 7b3a20b260
vasild:
ACK 7b3a20b260
Tree-SHA512: 0406dfcec180152d4f9ed07cbc2f406ad739b41f9c9cb38f8c75159c15d9d8a9a5c7526765966e40d695d265c178f6a80152e7edf82da344a65e55938dddb63d
8169fc4e73 qt, refactor: Fix code styling of moved InitExecutor class (Hennadii Stepanov)
c82165a557 qt, refactor: Move InitExecutor class into its own module (Hennadii Stepanov)
dbcf56b6c6 scripted-diff: Rename BitcoinCore class to InitExecutor (Hennadii Stepanov)
19a1d00831 qt: Add BitcoinCore::m_thread member (Hennadii Stepanov)
Pull request description:
This PR makes the `BitcoinCore` class reusable, i.e., it can be used by the widget-based GUI or by the [QML-based](https://github.com/bitcoin-core/gui-qml/tree/main/src/qml) one, and it makes the divergence between these two repos minimal.
The small benefit to the current branch is more structured code.
Actually, this PR is ported from https://github.com/bitcoin-core/gui-qml/pull/10.
The example of the re-using of the `BitcoinCore` class is https://github.com/bitcoin-core/gui-qml/pull/11.
ACKs for top commit:
laanwj:
ACK 8169fc4e73
ryanofsky:
Code review ACK 8169fc4e73. Only change is switching from `m_executor` from pointer to optional type (thanks for update!)
Tree-SHA512: a0552c32d26d9acf42921eb12bcdf68f02d52f7183c688c43257b1a58679f64e45f193ee2d316850c7f0f516561e17abe989fe545bfa05e158ad3f4c66d19bca
3e33d170cc Add ipc::Context and ipc::capnp::Context structs (Russell Yanofsky)
Pull request description:
These are currently empty structs but they will be used to pass some function and object pointers from bitcoin application code to IPC hooks that run, for example, when a remote object is created or destroyed, or a new process is created.
---
This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/projects/10). The commit was first part of larger PR #10102.
ACKs for top commit:
ariard:
Code Review ACK 3e33d170
Tree-SHA512: fd949fae5f1a973d39cb97f2745821ab2f62b98e166e53bc2801f97dcde988e18faaaaa0ffc2a82c170938b3a18078b6162fa35460e6e7c635e681b3c9e5b0a6
007910388b [Refactor] Rename scriptPubKey -> exec_script (sanket1729)
Pull request description:
Rename scriptPubKey to witness_script in ExecuteWitnessScript() function to correctly reflect which script is being executed.
For example in segwitv0, this scriptPubKey refers to the script of the form `OP_0 <script_hash>`, but witness_script refers to the script that actually hashes to the `script_hash`.
If there is a reason why it's named this way, I would love to know
ACKs for top commit:
MarcoFalke:
review ACK 007910388b🖖
theStack:
ACK 007910388b
lsilva01:
Code Review 007910388b ACK
Tree-SHA512: 768e10e656b60b1293beb560fb7adbc2c1495e6db1f54f0c2c63109692ae0c579c856b194b33f72afd0d332159a9796c0e2bd99b79ea5c4b1803469a81301fd6
cdd51e8ee1 torcontrol: Resolve Tor control plane address (Adrian-Stefan Mares)
Pull request description:
Closes https://github.com/bitcoin/bitcoin/issues/22236
This PR forces the Tor control plane address to be resolved before a connection attempt is made, similar to how the `-proxy` / `-onion` address is resolved.
The use case for this change is that the control plane may not have a stable address - in a containerized environment perhaps.
ACKs for top commit:
jonatack:
ACK cdd51e8ee1 tested various configurations on signet with this branch versus master
laanwj:
LGTM ACK cdd51e8ee1
theStack:
ACK cdd51e8ee1🪐
prayank23:
ACK cdd51e8ee1
Tree-SHA512: 5335cfcb89089a2acd6d02b88c2022dec60bb74388a99187c901c1c35d32896814d5f81df55c053953276c51fcec263c6ddadd068316f8e428b841bd599fc21e
2b19f3443e RPC/blockchain: getblockchaininfo: Include versionbits signalling details during LOCKED_IN (Luke Dashjr)
Pull request description:
While the signal has no effect during `LOCKED_IN`, the bit is still defined and recommended for measuring uptake. Makes sense to expose statistics too.
ACKs for top commit:
prayank23:
ACK 2b19f3443e
Sjors:
tACK 2b19f34
theStack:
Tested ACK 2b19f3443e
MarcoFalke:
review-only ACK 2b19f3443e
Tree-SHA512: a9bb5adb21992586119cbb5f87e5348eabcab11d5a3bf769b00b69e466589a669846e503f8384fa8927fd77da0c2d64a54f13a7a55a62980046d70f8255ddf47
20edf4bcf6 rpc: Return block time in getblockchaininfo (João Barbosa)
Pull request description:
Return tip time in `getblockchaininfo`, for some use cases this can save a call to `getblock`.
ACKs for top commit:
naumenkogs:
ACK 20edf4bcf6
theStack:
re-ACK 20edf4bcf6
0xB10C:
ACK 20edf4bcf6
kristapsk:
ACK 20edf4bcf6
Zero-1729:
re-ACK 20edf4bcf6
Tree-SHA512: 29a920cfff1ef53e0af601c3f93f8f9171f3be47fc84b0fa293cb865b824976e8c1510b17b27d17daf0b8e658dd77d9dc388373395f0919fc4a23cd5019642d5
f036dfbb69 [addrman] Remove unused test_before_evict argument from Good() (John Newbery)
Pull request description:
This has never been used in the public interface method since it was
introduced in #9037.
ACKs for top commit:
lsilva01:
Tested ACK f036dfbb69 on Ubuntu 20.04.
theStack:
Code-review ACK f036dfbb69
Tree-SHA512: 98145d9596b4ae1f354cfa561be1a54c6b8057c920e0ac3d4c1d42c9326b2dad2d44320f4171bb701d97088b216760cca8017b84c8b5dcd2b1dc8f158f28066d
faa54e3757 Move pblocktree global to BlockManager (MarcoFalke)
fa27f03b49 Move LoadBlockIndexDB to BlockManager (MarcoFalke)
Pull request description:
The block tree db is used within BlockManager to write and read the block index, so make the db global a member variable of BlockManager.
ACKs for top commit:
jamesob:
ACK faa54e3757 ([`jamesob/ackr/22371.1.MarcoFalke.move_pblocktree_global_t`](https://github.com/jamesob/bitcoin/tree/ackr/22371.1.MarcoFalke.move_pblocktree_global_t))
theStack:
re-ACK faa54e3757🥧
ryanofsky:
Code review ACK faa54e3757. I was thinking this looked like a change Carl would like, so no surprised he [Mega-acked](https://github.com/bitcoin/bitcoin/pull/22371#pullrequestreview-696450475)
Tree-SHA512: 1b7badbf503d53f5d4dbd9ed8f2e5c1ebfe48102665197048cc9e37bc87b5cec5f2277f3aae9f73a1095bfe879b19d288286ca3daa28031f5f1b64b1184439a9
fa621ededd refactor: Pass script verify flags as uint32_t (MarcoFalke)
Pull request description:
The flags are cast to unsigned in the interpreter anyway, so avoid the confusion (and fuzz crashes) by just passing them as unsigned from the beginning.
Also, the flags are often inverted bit-wise with the `~` operator, which also works on signed integers, but might cause confusion as the sign bit is flipped.
Fixes#22233
ACKs for top commit:
theStack:
Concept and code review ACK fa621ededd
kristapsk:
ACK fa621ededd
jonatack:
ACK fa621ededd
Tree-SHA512: ea0720f32f823fa7f075309978672aa39773c6019d12b6c1c9d611fc1983a76115b7fe2a28d50814673bb6415c311ccc05b99d6e871575fb6900faf75ed17769
9b85a5e2f7 tests: Test for dumpwallet lock order issue (Andrew Chow)
25d99e6511 Reorder dumpwallet so that cs_main functions go first (Andrew Chow)
Pull request description:
When a wallet is loaded which has an unconfirmed transaction in the mempool, it will end up establishing the lock order of cs_wallet -> cs_main -> cs_KeyStore. If `dumpwallet` is used on this wallet, then a lock order of cs_wallet -> cs_KeyStore -> cs_main will be used, which causes a lock order assertion. This PR fixes this by reordering `dumpwallet` and `GetKeyBirthTimes` (only used by `dumpwallet`). Specifically, in both functions, the function calls which lock cs_main are done prior to locking cs_KeyStore. This avoids the lock order issue.
Additionally, I have added a test case to `wallet_dump.py`. Of course testing this requires `--enable-debug`.
Fixes#22489
ACKs for top commit:
MarcoFalke:
review ACK 9b85a5e2f7🎰
ryanofsky:
Code review ACK 9b85a5e2f7. Nice to reduce lock scope, and good test!
prayank23:
tACK 9b85a5e2f7
lsilva01:
Tested ACK 9b85a5e2f7 under the same conditions reported in issue #22489 and the `dumpwallet` command completed successfully.
Tree-SHA512: d370a8f415ad64ee6a538ff419155837bcdbb167e3831b06572562289239028c6b46d80b23d227286afe875d9351f3377574ed831549ea426fb926af0e19c755
5a77abd4e6 [style] Clean up BroadcastTransaction() (John Newbery)
7282d4c036 [test] Allow rebroadcast for same-txid-different-wtxid transactions (glozow)
cd48372b67 [mempool] Allow rebroadcast for same-txid-different-wtxid transactions (John Newbery)
847b6ed48d [test] Test transactions are not re-added to unbroadcast set (Duncan Dean)
2837a9f1ea [mempool] Only add a transaction to the unbroadcast set when it's added to the mempool (John Newbery)
Pull request description:
1. Only add a transaction to the unbroadcast set when it's added to the mempool
Currently, if BroadcastTransaction() is called to rebroadcast a
transaction (e.g. by ResendWalletTransactions()), then we add the
transaction to the unbroadcast set. That transaction has already been
broadcast in the past, so peers are unlikely to request it again,
meaning RemoveUnbroadcastTx() won't be called and it won't be removed
from m_unbroadcast_txids.
Net processing will therefore continue to attempt rebroadcast for the
transaction every 10-15 minutes. This will most likely continue until
the node connects to a new peer which hasn't yet seen the transaction
(or perhaps indefinitely).
Fix by only adding the transaction to the broadcast set when it's added to the mempool.
2. Allow rebroadcast for same-txid-different-wtxid transactions
There is some slightly unexpected behaviour when:
- there is already transaction in the mempool (the "mempool tx")
- BroadcastTransaction() is called for a transaction with the same txid
as the mempool transaction but a different witness (the "new tx")
Prior to this commit, if BroadcastTransaction() is called with
relay=true, then it'll call RelayTransaction() using the txid/wtxid of
the new tx, not the txid/wtxid of the mempool tx. For wtxid relay peers,
in SendMessages(), the wtxid of the new tx will be taken from
setInventoryTxToSend, but will then be filtered out from the vector of
wtxids to announce, since m_mempool.info() won't find the transaction
(the mempool contains the mempool tx, which has a different wtxid from
the new tx).
Fix this by calling RelayTransaction() with the wtxid of the mempool
transaction in this case.
The third commit is a comment/whitespace only change to tidy up the BroadcastTransaction() function.
ACKs for top commit:
duncandean:
reACK 5a77abd
naumenkogs:
ACK 5a77abd4e6
theStack:
re-ACK 5a77abd4e6
lsilva01:
re-ACK 5a77abd4e6
Tree-SHA512: d1a46d32a9f975220e5b432ff6633fac9be01ea41925b4958395b8d641680500dc44476b12d18852e5b674d2d87e4d0160b4483e45d3d149176bdff9f4dc8516
5730a43703 test: Add functional test for AddrFetch connections (Martin Zumsande)
c34ad3309f net, rpc: Enable AddrFetch connections for functional testing (Martin Zumsande)
533500d907 p2p: Add timeout for AddrFetch peers (Martin Zumsande)
b6c5d1e450 p2p: AddrFetch - don't disconnect on self-announcements (Martin Zumsande)
Pull request description:
AddrFetch connections (old name: oneshots) are intended to be short-lived connections on which we ask a peer for addresses via `getaddr` and disconnect after receiving them.
This is done by disconnecting after receiving the first `addr`. However, it is no longer working as intended, because nowadays, the first `addr` a typical bitcoin core node sends is its self-announcement.
So we'll disconnect before the peer gets a chance to answer our `getaddr`.
I checked that this affects both `-seednode` peers specified manually, and DNS seeds when AddrFetch is used as a fallback if DNS doesn't work for us.
The current behavior of getting peers via AddrFetch when starting with an empty addrman would be to connect to the peer, receive its self-announcement and add it to addrman, disconnect, reconnect to the same peer again as a full outbound (no other addresses in addrman) and then receive more `addr`. This is silly and not in line with AddrFetch peer being intended to be short-lived peers.
Fix this by only disconnecting after receiving an `addr` message of size > 1.
[Edit] As per review discussion, this PR now also adds a timeout after which we disconnect if we haven't received any suitable `addr`, and a functional test.
ACKs for top commit:
amitiuttarwar:
reACK 5730a43703
naumenkogs:
ACK 5730a43703
jnewbery:
ACK 5730a43703
Tree-SHA512: 8a81234f37e827705138eb254223f7f3b3bf44a06cb02126fc7990b0d231b9bd8f07d38d185cc30d55bf35548a6fdc286b69602498d875b937e7c58332158bf9
Now that m_recent_confirmed_transactions is owned by PeerManagerImpl,
and PeerManagerImpl's lifetime is managed by the node context, we can
just default initialize m_recent_confirmed_transactions during object
initialization. We can also remove the unique_ptr indirection.
Now that recentRejects is owned by PeerManagerImpl, and
PeerManagerImpl's lifetime is managed by the node context, we can just
default initialize recentRejects during object initialization. We can
also remove the unique_ptr indirection.
Instead of deserializing addresses, placing them in the buckets, and
then removing them if they're invalid, check first and don't place in
the buckets if they're invalid.
facd56750c scripted-diff: Revert "fuzz: Add Temporary debug assert for oss-fuzz issue" (MarcoFalke)
Pull request description:
No longer needed, as it wouldn't help to debug this issue. See https://github.com/bitcoin/bitcoin/pull/22472#issuecomment-882692900
ACKs for top commit:
fanquake:
ACK facd56750c
Tree-SHA512: 13352b3529c43d6e65ab127134b32158d3072dc2fbbb326fea9adfeada5a8610d0477ea75748b8b68e7abb3b9869a989df3a3169e92bdd458053d64bae6ed379
DEBUG_LOCKORDER expects cs_wallet, cs_main, and cs_KeyStore to be
acquired in that order. However dumpwallet would take these in the order
cs_wallet, cs_KeyStore, cs_main. So when configured with
`--enable-debug`, it is possible to hit the lock order assertion when
using dumpwallet.
To fix this, cs_wallet and cs_KeyStore are no longer locked at the same
time. Instead cs_wallet will be locked first. Then the functions which
lock cs_main will be run. Lastly cs_KeyStore will be locked afterwards.
This avoids the lock order issue.
Furthermore, since GetKeyBirthTimes (only used by dumpwallet) also uses
a function that locks cs_main, and itself also locks cs_KeyStore, the
same reordering is done here.
816f29eab2 addrman: detect on-disk corrupted nNew and nTried during unserialization (Vasil Dimov)
Pull request description:
Negative `nNew` or `nTried` are not possible during normal operation.
So, if we read such values during unserialize, report addrman
corruption.
Fixes https://github.com/bitcoin/bitcoin/issues/22450
ACKs for top commit:
MarcoFalke:
cr ACK 816f29eab2
jonatack:
ACK 816f29eab2
lsilva01:
Code Review ACK 816f29eab2. This change provides a more accurate description of the error.
Tree-SHA512: 01bdd72d2d86a0ef770a319fee995fd1e147b24a8db84ddb8cd121688e7f94fed73fddc0084758e7183c4f8d08e971f0b1b224f5adb10928a5aa4dbbc8709d74
a4bcd687c9 Improve tests using statistics (John Newbery)
f424d601e1 Add logging and addr rate limiting statistics (Pieter Wuille)
b4ece8a1cd Functional tests for addr rate limiting (Pieter Wuille)
5648138f59 Randomize the order of addr processing (Pieter Wuille)
0d64b8f709 Rate limit the processing of incoming addr messages (Pieter Wuille)
Pull request description:
The rate at which IP addresses are rumoured (through ADDR and ADDRV2 messages) on the network seems to vary from 0 for some non-participating nodes, to 0.005-0.025 addr/s for recent Bitcoin Core nodes. However, the current codebase will happily accept and process an effectively unbounded rate from attackers. There are measures to limit the influence attackers can have on the addrman database (bucket restrictions based on source IPs), but still - there is no need to permit them to feed us addresses at a rate that's orders of magnitude larger than what is common on the network today, especially as it will cause us to spam our peers too.
This PR implements a [token bucket](https://en.wikipedia.org/wiki/Token_bucket) based rate limiter, allowing an average of 0.1 addr/s per connection, with bursts up to 1000 addresses at once. Whitelisted peers as well as responses to GETADDR requests are exempt from the limit. New connections start with 1 token, so as to not interfere with the common practice of peers' self-announcement.
ACKs for top commit:
laanwj:
ACK a4bcd687c9
vasild:
ACK a4bcd687c9
jnewbery:
ACK a4bcd687c9
jonatack:
ACK a4bcd687c9
Tree-SHA512: b757de76ad78a53035b622944c4213b29b3b55d3d98bf23585afa84bfba10808299d858649f92269a16abfa75eb4366ea047eae3216f7e2f6d3c455782a16bea
8465978f23 Make IsSegWitOutput return true for taproot outputs (Pieter Wuille)
Pull request description:
This fixes a bug: currently `utxoupdatepsbt` will not fill in UTXO data for PSBTs spending taproot outputs.
ACKs for top commit:
achow101:
Code Review ACK 8465978f23
jonatack:
ACK 8465978f23
meshcollider:
utACK 8465978f23
Tree-SHA512: 2f8f873450bef4b5a4ce5962a231297b386c6b1445e69ce5f36ab28eca7343be3a11bc09c38534b0f75e6f99ba15d78d3ba5d484f6c63e5a9775e1f3f55a74e0
a2aca207b1 Move implementations of non-template fuzz helpers (Sriram)
Pull request description:
There are 78 cpp files that include `util.h` (`grep -iIr "#include <test/fuzz/util.h>" src/test/fuzz | wc -l`). Modifying the implementation of a fuzz helper in `src/test/fuzz/util.h` will cause all fuzz tests to be recompiled. Keeping the declarations of these non-template fuzz helpers in `util.h` and moving their implementations to `util.cpp` will skip the redundant recompilation of all the fuzz tests, and builds these helpers only once in `util.cpp`.
Functions moved from `util.h` to `util.cpp`:
- `ConsumeTxMemPoolEntry`
- `ContainsSpentInput`
- `ConsumeNetAddr`
- Methods of `FuzzedFileProvider::(open, read, write, seek, close)`
ACKs for top commit:
MarcoFalke:
review ACK a2aca207b1🍂
Tree-SHA512: e7037ebb86d0fc56048e4f3d8733eefc21da11683b09d2b22926bda410719628d89c52ddd9b4c18aa243607a66fdb4d13a63e62ca010e66b3ec9174fd18107f0
5012a7912e Test that descriptor wallet upgrade does nothing (Andrew Chow)
48bd7d3b77 Change ScriptPubKeyMan::Upgrade to default to return true (Andrew Chow)
Pull request description:
When adding a new ScriptPubKeyMan, it's likely that there will be nothing for `Upgrade` to do. If it is called (via `upgradewallet`), then it should do nothing, successfully. This PR changes the default `ScriptPubKeyMan::Upgrade` function so that it returns a success instead of failure when doing nothing.
Fixes#22460
ACKs for top commit:
jonatack:
ACK 5012a7912e
meshcollider:
utACK 5012a7912e
Tree-SHA512: 578c6521e997f7bb5cc44be2cfe9e0a760b6bd4aa301026a6b8b3282e8757473e4cb9f68b2e79dacdc2b42dddae718450072e0a38817df205dfea177a74d7f3d
fb7be92b09 Mark print-% target as phony. (Dmitry Goncharov)
Pull request description:
.PHONY does not take patterns (such as print-%) as prerequisites.
Have print-% depend on force and mark force as phony.
This change ensures print-% rule works even when there is a file that matches the target.
```
$ # on master
$ make print-host
host=x86_64-pc-linux-gnu
$ touch print-host
$ make print-host
make: 'print-host' is up to date.
$
$ git co mark_print_as_phony
Switched to branch 'mark_print_as_phony'
$ make print-host
host=x86_64-pc-linux-gnu
$ touch force
$ make print-host
host=x86_64-pc-linux-gnu
```
ACKs for top commit:
hebasto:
ACK fb7be92b09, tested on Linux Mint 20.2 (x86_64).
Tree-SHA512: b89ae66aa8c7aa6a7ab5f0956f9eb3b3ef9d56994b60dc2a97d498d4c1bba537845c190723e8a10310280b1b35df2cd935cc30aeb76735cac2dc621ad7823772
3c4c8e79ba build: Add -Werror=implicit-fallthrough compile flag (Hennadii Stepanov)
014110c47d Use C++17 [[fallthrough]] attribute, and drop -Wno-implicit-fallthrough (Hennadii Stepanov)
Pull request description:
ACKs for top commit:
fanquake:
ACK 3c4c8e79ba - looks ok to me now. Checked that warnings occur in our code & leveldb by removing a `[[fallthrough]]` or `FALLTHROUGH_INTENDED`.
jarolrod:
ACK 3c4c8e79ba
theStack:
ACK 3c4c8e79ba
Tree-SHA512: 4dce91f0f26b8a3de09bd92bb3d7e1995e078e3a8b3ff861c4fbf6c0b32b2327d063633b07b89c4aa94a1141d7f78d46d9d43ab8df865273e342693ad30645b6
After feedback from Russ, I realized that there are some extraneous assumeutxo methods
that are not necessary and probably just overly confusing. These include
- `Validated*()`
- `IsBackgroundIBD()`
and they can be removed.
Add an upwards reference to chainstate instances to the owning
ChainstateManager. This is necessary because there are a number
of `this_chainstate == chainman.ActiveChainstate()` checks that
will happen (as a result of assumeutxo) in functions that otherwise
don't have an easily-accessible reference to the chainstate's
ChainManager.
While limitations on the influence of attackers on addrman already
exist (affected buckets are restricted to a subset based on incoming
IP / network group), there is no reason to permit them to let them
feed us addresses at more than a multiple of the normal network
rate.
This commit introduces a "token bucket" rate limiter for the
processing of addresses in incoming ADDR and ADDRV2 messages.
Every connection gets an associated token bucket. Processing an
address in an ADDR or ADDRV2 message from non-whitelisted peers
consumes a token from the bucket. If the bucket is empty, the
address is ignored (it is not forwarded or processed). The token
counter increases at a rate of 0.1 tokens per second, and will
accrue up to a maximum of 1000 tokens (the maximum we accept in a
single ADDR or ADDRV2). When a GETADDR is sent to a peer, it
immediately gets 1000 additional tokens, as we actively desire many
addresses from such peers (this may temporarily cause the token
count to exceed 1000).
The rate limit of 0.1 addr/s was chosen based on observation of
honest nodes on the network. Activity in general from most nodes
is either 0, or up to a maximum around 0.025 addr/s for recent
Bitcoin Core nodes. A few (self-identified, through subver) crawler
nodes occasionally exceed 0.1 addr/s.
If a ScriptPubKeyMan does not implement Upgrade, then using upgraewallet
will fail unexpectedly. By changing the default to return true, then
this error can be avoided. This is still correct because a successful
upgrade can be that nothing happened.
7593b06bd1 test: ensure I2P addresses are relayed (Vasil Dimov)
e7468139a1 test: make CAddress in functional tests comparable (Vasil Dimov)
33e211d2a4 test: implement ser/unser of I2P addresses in functional tests (Vasil Dimov)
86742811ce test: use NODE_* constants instead of magic numbers (Vasil Dimov)
ba45f02708 net: relay I2P addresses even if not reachable (by us) (Vasil Dimov)
Pull request description:
Nodes that can reach the I2P network (have set `-i2psam=`) will relay
I2P addresses even without this patch. However, nodes that can't reach
the I2P network will not. This was done as a precaution in
https://github.com/bitcoin/bitcoin/pull/20119 before anybody could
connect to I2P because then, for sure, it would have been useless.
Now, however, we have I2P support and a bunch of I2P nodes, so get all
nodes on the network to relay I2P addresses to help with propagation,
similarly to what we do with Tor addresses.
ACKs for top commit:
jonatack:
ACK 7593b06bd1
naumenkogs:
ACK 7593b06bd1.
laanwj:
Code review ACK 7593b06bd1
kristapsk:
ACK 7593b06bd1. Code looks correct, tested that functional test suite passes and also that `test/functional/p2p_addrv2_replay.py` fails if I undo changes in `IsRelayable()`.
Tree-SHA512: c9feec4a9546cc06bc2fec6d74f999a3c0abd3d15b7c421c21fcf2d610eb94611489e33d61bdcd5a4f42041a6d84aa892f7ae293b0d4f755309a8560b113b735
b1d905c225 p2p: earlier continuation when no remaining eviction candidates (Vasil Dimov)
c9e8d8f9b1 p2p: process more candidates per protection iteration (Jon Atack)
02e411ec45 p2p: iterate eviction protection only on networks having candidates (Jon Atack)
5adb064574 bench: add peer eviction protection benchmarks (Jon Atack)
566357f8f7 refactor: move GetRandomNodeEvictionCandidates() to test utilities (Jon Atack)
Pull request description:
This follow-up to #21261 improves `ProtectEvictionCandidatesByRatio()` for better performance.
Benchmarks are added; the performance improvement is between 2x and 5x for the benchmarked cases (CPU 2.50GHz, Turbo off, performance mode, Debian Clang 11 non-debug build).
```
$ ./src/bench/bench_bitcoin -filter="EvictionProtection*.*"
```
The refactored code is well-covered by existing unit tests and also a fuzzer.
- `$ ./src/test/test_bitcoin -t net_peer_eviction_tests`
- `$ FUZZ=node_eviction ./src/test/fuzz/fuzz ../qa-assets/fuzz_seed_corpus/node_eviction`
ACKs for top commit:
klementtan:
Tested and code review ACK b1d905c2.
vasild:
ACK b1d905c225
jarolrod:
ACK b1d905c225
Tree-SHA512: a3a6607b9ea2fec138da9780c03f63e177b6712091c5a3ddc3804b896a7585216446310280791f5e20cc023d02d2f03a4139237e12b5c1d7f2a1fa1011610e96
Negative `nNew` or `nTried` are not possible during normal operation.
So, if we read such values during unserialize, report addrman
corruption.
Fixes https://github.com/bitcoin/bitcoin/issues/22450
ceb7b35a39 refactor: move UpdateTip into CChainState (James O'Beirne)
4abf0779d6 refactor: no mempool arg to GetCoinsCacheSizeState (James O'Beirne)
46e3efd1e4 refactor: move UpdateMempoolForReorg into CChainState (James O'Beirne)
617661703a validation: make CChainState::m_mempool optional (James O'Beirne)
Pull request description:
Make `CChainState::m_mempool` optional by making it a pointer instead of a reference. This will allow a simplification to assumeutxo semantics (see https://github.com/bitcoin/bitcoin/pull/15606#pullrequestreview-692965905) and help facilitate the `-nomempool` option.
ACKs for top commit:
jnewbery:
ACK ceb7b35a39
naumenkogs:
ACK ceb7b35a39
ryanofsky:
Code review ACK ceb7b35a39 (just minor style and test tweaks since last review)
lsilva01:
Code review ACK and tested on Signet ACK ceb7b35a39
MarcoFalke:
review ACK ceb7b35a39😌
Tree-SHA512: cc445ad33439d5918cacf80a6354eea8f3d33bb7719573ed5b970fad1a0dab410bcd70be44c862b8aba1b71263b82d79876688c553e339362653dfb3d8ec81e6
fa5658ed07 Use DeploymentEnabled to hide VB deployments (MarcoFalke)
fa11fecf0d doc: Move buried deployment doc to the enum that enumerates them (MarcoFalke)
Pull request description:
Plus a doc commit.
ACKs for top commit:
jnewbery:
utACK fa5658ed07
ajtowns:
utACK fa5658ed07
Tree-SHA512: 2aeceee0674feb603d76656eff40695b7d7305de309f837bbb6a8c1dbb1d0b962b741f06ab7b9a8b1dbd1964c9c0c9aa5dc9588fd8e6d896e620b69e08eedbaa
be8d9c262f Merge bitcoin-core/secp256k1#965: gen_context: Don't use any ASM
aeece44599 gen_context: Don't use any ASM
7688a4f13a Merge bitcoin-core/secp256k1#963: "Schnorrsig API overhaul" fixups
90e83449b2 ci: Add C++ test
f698caaff6 Use unsigned char consistently for byte arrays
b5b8e7b719 Don't declare constants twice
769528f307 Don't use string literals for char arrays without NUL termination
2cc3cfa583 Fix -Wmissing-braces warning in clang
0440945fb5 Merge #844: schnorrsig API overhaul
ec3aaa5014 Merge #960: tests_exhaustive: check the result of secp256k1_ecdsa_sign
a1ee83c654 tests_exhaustive: check the result of secp256k1_ecdsa_sign
253f90cdeb Merge bitcoin-core/secp256k1#951: configure: replace AC_PATH_PROG to AC_CHECK_PROG
446d28d9de Merge bitcoin-core/secp256k1#944: Various improvements related to CFLAGS
0302138f75 ci: Make compiler warning into errors on CI
b924e1e605 build: Ensure that configure's compile checks default to -O2
7939cd571c build: List *CPPFLAGS before *CFLAGS like on the compiler command line
595e8a35d8 build: Enable -Wcast-align=strict warning
07256267ff build: Use own variable SECP_CFLAGS instead of touching user CFLAGS
4866178dfc Merge bitcoin-core/secp256k1#955: Add random field multiply/square tests
75ce488c2a Merge bitcoin-core/secp256k1#959: tests: really test the non-var scalar inverse
41ed13942b tests: really test the non-var scalar inverse
5f6ceafcfa schnorrsig: allow setting MSGLEN != 32 in benchmark
fdd06b7967 schnorrsig: add tests for sign_custom and varlen msg verification
d8d806aaf3 schnorrsig: add extra parameter struct for sign_custom
a0c3fc177f schnorrsig: allow signing and verification of variable length msgs
5a8e4991ad Add secp256k1_tagged_sha256 as defined in BIP-340
b6c0b72fb0 schnorrsig: remove noncefp args from sign; add sign_custom function
bdf19f105c Add random field multiply/square tests
8ae56e33e7 Merge #879: Avoid passing out-of-bound pointers to 0-size memcpy
a4642fa15e configure: replace AC_PATH_PROG to AC_CHECK_PROG
1758a92ffd Merge #950: ci: Add ppc64le build
c58c4ea470 ci: Add ppc64le build
7973576f6e Merge #662: Add ecmult_gen, ecmult_const and ecmult to benchmark
8f879c2887 Fix array size in bench_ecmult
2fe1b50df1 Add ecmult_gen, ecmult_const and ecmult to benchmark
593e6bad9c Clean up ecmult_bench to make space for more benchmarks
50f3367712 Merge #947: ci: Run PRs on merge result even for i686
a35fdd3478 ci: Run PRs on merge result even for i686
442cee5baf schnorrsig: add algolen argument to nonce_function_hardened
df3bfa12c3 schnorrsig: clarify result of calling nonce_function_bip340 without data
99e8614812 README: mention schnorrsig module
3dc8c072b6 Merge #846: ci: Run ASan/LSan and reorganize sanitizer and Valgrind jobs
02dcea1ad9 ci: Make test iterations configurable and tweak for sanitizer builds
489ff5c20a tests: Treat empty SECP2561_TEST_ITERS as if it was unset
fcfcb97e74 ci: Simplify to use generic wrapper for QEMU, Valgrind, etc
de4157f13a ci: Run ASan/LSan and reorganize sanitizer and Valgrind jobs
399722a63a Merge #941: Clean up git tree
09b3bb8648 Clean up git tree
bf0ac46066 Merge #930: Add ARM32/ARM64 CI
202a030f7d Merge #850: add `secp256k1_ec_pubkey_cmp` method
1e78c18d5b Merge bitcoin-core/secp256k1#940: contrib: Explain explicit header guards
69394879b6 Merge #926: secp256k1.h: clarify that by default arguments must be != NULL
6eceec6d56 add `secp256k1_xonly_pubkey_cmp` method
0d9561ae87 add `secp256k1_ec_pubkey_cmp` method
22a9ea154a contrib: Explain explicit header guards
6c52ae8724 Merge #937: Have ge_set_gej_var, gej_double_var and ge_set_all_gej_var initialize all fields of their outputs.
185a6af227 Merge #925: changed include statements without prefix 'include/'
14c9739a1f tests: Improve secp256k1_ge_set_all_gej_var for some infinity inputs
4a19668c37 tests: Test secp256k1_ge_set_all_gej_var for all infinity inputs
3c90bdda95 change local lib headers to be relative for those pointing at "include/" dir
45b6468d7e Have secp256k1_ge_set_all_gej_var initialize all fields. Previous behaviour would not initialize r->y values in the case where infinity is passed in. Furthermore, the previous behaviour wouldn't initialize anything in the case where all inputs were infinity.
31c0f6de41 Have secp256k1_gej_double_var initialize all fields. Previous behaviour would not initialize r->x and r->y values in the case where infinity is passed in.
dd6c3de322 Have secp256k1_ge_set_gej_var initialize all fields. Previous behaviour would not initialize r->x and r->y values in the case where infinity is passed in.
d0bd2693e3 Merge bitcoin-core/secp256k1#936: Fix gen_context/ASM build on ARM
8bbad7a18e Add asm build to ARM32 CI
7d65ed5214 Add ARM32/ARM64 CI
c8483520c9 Makefile.am: Don't pass a variable twice
2161f31785 Makefile.am: Honor config when building gen_context
99f47c20ec gen_context: Don't use external ASM because it complicates the build
98e0358d29 Merge #933: Avoids a missing brace warning in schnorrsig/tests_impl.h on old compilers
99e2d5be0d Avoids a missing brace warning in schnorrsig/tests_impl.h on old compilers.
34388af6b6 Merge #922: Add mingw32-w64/wine CI build
7012a188e6 Merge #928: Define SECP256K1_BUILD in secp256k1.c directly.
ed5a199bed tests: fopen /dev/urandom in binary mode
ae9e648526 Define SECP256K1_BUILD in secp256k1.c directly.
4dc37bf81b Add mingw32-w64/wine CI build
0881633dfd secp256k1.h: clarify that by default arguments must be != NULL
9570f674cc Avoid passing out-of-bound pointers to 0-size memcpy
git-subtree-dir: src/secp256k1
git-subtree-split: be8d9c262f46309d9b4165b0498b71d704aba8fe
Moved implementations of `ConsumeTxMemPoolEntry`, `ContainsSpentInput`, `ConsumeNetAddr`, and the methods(open, read, write, seek, close) of FuzzedFileProvider from test/fuzz/util.h to test/fuzz/util.cpp.
Since we now have multiple chainstate objects, only one of them is active at any given
time. An active chainstate has a mempool, but there's no point to others having one.
This change will simplify proposed assumeutxo semantics. See the discussion here:
https://github.com/bitcoin/bitcoin/pull/15606#pullrequestreview-692965905
Co-authored-by: Russell Yanofsky <russ@yanofsky.org>
4101ec9d2e doc: mention that we enforce port=0 in I2P (Vasil Dimov)
e0a2b390c1 addrman: reset I2P ports to 0 when loading from disk (Vasil Dimov)
41cda9d075 test: ensure I2P ports are handled as expected (Vasil Dimov)
4f432bd738 net: do not connect to I2P hosts on port!=0 (Vasil Dimov)
1f096f091e net: distinguish default port per network (Vasil Dimov)
aeac3bce3e net: change I2P seeds' ports to 0 (Vasil Dimov)
38f900290c net: change assumed I2P port to 0 (Vasil Dimov)
Pull request description:
_This is an alternative to https://github.com/bitcoin/bitcoin/pull/21514, inspired by https://github.com/bitcoin/bitcoin/pull/21514#issuecomment-815049933. They are mutually exclusive. Just one of them should be merged._
Change assumed ports for I2P to 0 (instead of the default 8333) as this is closer to what actually happens underneath with SAM 3.1 (https://github.com/bitcoin/bitcoin/pull/21514#issuecomment-812632520, https://github.com/bitcoin/bitcoin/pull/21514#issuecomment-816564719).
Don't connect to I2P peers with advertised port != 0 (we don't specify a port to our SAM 3.1 proxy and it always connects to port = 0).
Note, this change:
* Keeps I2P addresses with port != 0 in addrman and relays them to others via P2P gossip. There may be non-bitcoin-core-22.0 peers using SAM 3.2 and for them such addresses may be useful.
* Silently refuses to connect to I2P hosts with port != 0. This is ok for automatically chosen peers from addrman. Not so ok for peers provided via `-addnode` or `-connect` - a user who specifies `foo.b32.i2p:1234` (non zero port) may wonder why "nothing is happening".
Fixes#21389
ACKs for top commit:
laanwj:
Code review ACK 4101ec9d2e
jonatack:
re-ACK 4101ec9d2e per `git range-diff efff9c3 0b0ee03 4101ec9`, built with DDEBUG_ADDRMAN, did fairly extensive testing on mainnet both with and without a peers.dat / -dnsseeds=0 to test boostrapping.
Tree-SHA512: 0e3c019e1dc05e54f559275859d3450e0c735596d179e30b66811aad9d5b5fabe3dcc44571e8f7b99f9fe16453eee393d6e153454dd873b9ff14907d4e6354fe
2feec3ce31 net: don't bind on 0.0.0.0 if binds are restricted to Tor (Vasil Dimov)
Pull request description:
The semantic of `-bind` is to restrict the binding only to some address.
If not specified, then the user does not care and we bind to `0.0.0.0`.
If specified then we should honor the restriction and bind only to the
specified address.
Before this change, if no `-bind` is given then we would bind to
`0.0.0.0:8333` and to `127.0.0.1:8334` (incoming Tor) which is ok -
the user does not care to restrict the binding.
However, if only `-bind=addr:port=onion` is given (without ordinary
`-bind=`) then we would bind to `addr:port` _and_ to `0.0.0.0:8333` in
addition.
Change the above to not do the additional bind: if only
`-bind=addr:port=onion` is given (without ordinary `-bind=`) then bind
to `addr:port` (only) and consider incoming connections to that as Tor
and do not advertise it. I.e. a Tor-only node.
ACKs for top commit:
laanwj:
Code review ACK 2feec3ce31
jonatack:
utACK 2feec3ce31 per `git diff a004833 2feec3c`
hebasto:
ACK 2feec3ce31, tested on Linux Mint 20.1 (x86_64):
Tree-SHA512: a04483af601706da928958b92dc560f9cfcc78ab0bb9d74414636eed1c6f29ed538ce1fb5a17d41ed82c9c9a45ca94899d0966e7ef93da809c9bcdcdb1d1f040
This commit fixes some slightly unexpected behaviour when:
- there is already transaction in the mempool (the "mempool tx")
- BroadcastTransaction() is called for a transaction with the same txid
as the mempool transaction but a different witness (the "new tx")
Prior to this commit, if BroadcastTransaction() is called with
relay=true, then it'll call RelayTransaction() using the txid/wtxid of
the new tx, not the txid/wtxid of the mempool tx. For wtxid relay peers,
in SendMessages(), the wtxid of the new tx will be taken from
setInventoryTxToSend, but will then be filtered out from the vector of
wtxids to announce, since m_mempool.info() won't find the transaction
(the mempool contains the mempool tx, which has a different wtxid from
the new tx).
Fix this by calling RelayTransaction() with the wtxid of the mempool
transaction in this case.
Currently, if BroadcastTransaction() is called to rebroadcast a
transaction (e.g. by ResendWalletTransactions()), then we add the
transaction to the unbroadcast set. That transaction has already been
broadcast in the past, so peers are unlikely to request it again,
meaning RemoveUnbroadcastTx() won't be called and it won't be removed
from m_unbroadcast_txids.
Net processing will therefore continue to attempt rebroadcast for the
transaction every 10-15 minutes. This will most likely continue until
the node connects to a new peer which hasn't yet seen the transaction
(or perhaps indefinitely).
Fix by only adding the transaction to the broadcast set when it's added
to the mempool.
b7a8cd9963 [test] submit same txid different wtxid as mempool tx (glozow)
fdb48163bf [validation] distinguish same txid different wtxid in mempool (glozow)
Pull request description:
On master, if you submit a transaction with the same txid but different witness to the mempool, it thinks the transactions are the same. Users submitting through `BroadcastTransaction()` (i.e. `sendrawtransaction` or the wallet) don't get notified that there's a different transaction in the mempool, although it doesn't crash. Users submitting through `testmempoolaccept()` will get a "txn-already-in-mempool" error.
This PR simply distinguishes between `txn-already-in-mempool` and `txn-same-nonwitness-data-in-mempool`, without handling them differently: `sendrawtransaction` still will not throw, but `testmempoolaccept` will give you a different error.
I believe the intention of #19645 is to allow full swaps of transactions that have different witnesses but identical nonwitness data. Returning a different error message + adding a test was suggested: https://github.com/bitcoin/bitcoin/pull/19645#issuecomment-705109193 so this is that PR.
ACKs for top commit:
naumenkogs:
ACK b7a8cd9963
jnewbery:
Code review ACK b7a8cd9963
theStack:
Code-review ACK b7a8cd9963
darosior:
re-utACK b7a8cd9963
Tree-SHA512: 9c6591edaf8727ba5b4675977adb8cbdef7288584003b6cd659828032dc92d2ae915800a8ef8b6fdffe112c1b660df72297a3dcf2e2e3e1f959c6cb3678c63ee
This is a temporary change to convert I2P addresses that have propagated
with port 8333 to ones with port 0.
It would cause a problem some day if indeed some bitcoin software is
listening on port 8333 only and rejects connections to port 0 and we are
still using SAM 3.1 which only supports port 0. In this case we would
replace 8333 with 0 and try to connect to such nodes.
This commit should be included in 22.0 and be reverted before 23.0 is
released.
When connecting to an I2P host we don't specify destination port and it
is being forced to 0 by the SAM 3.1 proxy, so if we connect to the same
host on two different ports, that would be actually two connections to
the same service (listening on port 0).
Fixes https://github.com/bitcoin/bitcoin/issues/21389
* When accepting an I2P connection, assume the peer has port 0 instead
of the default 8333 (for mainnet). It is not being sent to us, so we
must assume something.
* When deriving our own I2P listen CService use port 0 instead of the
default 8333 (for mainnet). So that we later advertise it to peers
with port 0.
In the I2P protocol SAM 3.1 and older (we use 3.1) ports are not used,
so they are irrelevant. However in SAM 3.2 and newer ports are used and
from the point of view of SAM 3.2, a peer using SAM 3.1 seems to have
specified port=0.
4e44f5bac4 test: Correct outstanding -Werror=sign-compare errors (Ben Woosley)
Pull request description:
I'm unclear on why these aren't failing on CI, but they failed for me locally, e.g.:
```
In file included from /usr/local/include/boost/test/test_tools.hpp:46:
/usr/local/include/boost/test/tools/old/impl.hpp:107:17: error: comparison of integers of different signs: 'const unsigned int' and 'const int' [-Werror,-Wsign-compare]
return left == right;
~~~~ ^ ~~~~~
/usr/local/include/boost/test/tools/old/impl.hpp:130:16: note: in instantiation of function template specialization 'boost::test_tools::tt_detail::equal_impl<unsigned int, int>' requested here
return equal_impl( left, right );
^
/usr/local/include/boost/test/tools/old/impl.hpp:145:16: note: in instantiation of function template specialization 'boost::test_tools::tt_detail::equal_impl_frwd::call_impl<unsigned int, int>' requested here
return call_impl( left, right, left_is_array() );
^
/usr/local/include/boost/test/tools/old/impl.hpp:92:50: note: in instantiation of function template specialization 'boost::test_tools::tt_detail::equal_impl_frwd::operator()<unsigned int, int>' requested here
BOOST_PP_REPEAT( BOOST_TEST_MAX_PREDICATE_ARITY, IMPL_FRWD, _ )
^
/usr/local/include/boost/preprocessor/repetition/repeat.hpp:30:26: note: expanded from macro 'BOOST_PP_REPEAT'
^
/usr/local/include/boost/preprocessor/cat.hpp:22:32: note: expanded from macro 'BOOST_PP_CAT'
^
/usr/local/include/boost/preprocessor/cat.hpp:29:34: note: expanded from macro 'BOOST_PP_CAT_I'
^
<scratch space>:153:1: note: expanded from here
BOOST_PP_REPEAT_1
^
test/streams_tests.cpp:122:5: note: in instantiation of function template specialization 'boost::test_tools::tt_detail::check_frwd<boost::test_tools::tt_detail::equal_impl_frwd, unsigned int, int>' requested here
BOOST_CHECK_EQUAL(varint, 54321);
^
/usr/local/include/boost/test/tools/old/impl.hpp:107:17: error: comparison of integers of different signs: 'const unsigned long long' and 'const long' [-Werror,-Wsign-compare]
return left == right;
~~~~ ^ ~~~~~
/usr/local/include/boost/test/tools/old/impl.hpp:130:16: note: in instantiation of function template specialization 'boost::test_tools::tt_detail::equal_impl<unsigned long long, long>' requested here
return equal_impl( left, right );
^
/usr/local/include/boost/test/tools/old/impl.hpp:145:16: note: in instantiation of function template specialization 'boost::test_tools::tt_detail::equal_impl_frwd::call_impl<unsigned long long, long>' requested here
return call_impl( left, right, left_is_array() );
^
/usr/local/include/boost/test/tools/old/impl.hpp:92:50: note: in instantiation of function template specialization 'boost::test_tools::tt_detail::equal_impl_frwd::operator()<unsigned long long, long>' requested here
BOOST_PP_REPEAT( BOOST_TEST_MAX_PREDICATE_ARITY, IMPL_FRWD, _ )
^
/usr/local/include/boost/preprocessor/repetition/repeat.hpp:30:26: note: expanded from macro 'BOOST_PP_REPEAT'
^
/usr/local/include/boost/preprocessor/cat.hpp:22:32: note: expanded from macro 'BOOST_PP_CAT'
^
/usr/local/include/boost/preprocessor/cat.hpp:29:34: note: expanded from macro 'BOOST_PP_CAT_I'
^
<scratch space>:161:1: note: expanded from here
BOOST_PP_REPEAT_1
^
test/serfloat_tests.cpp:41:5: note: in instantiation of function template specialization 'boost::test_tools::tt_detail::check_frwd<boost::test_tools::tt_detail::equal_impl_frwd, unsigned long long, long>' requested here
BOOST_CHECK_EQUAL(TestDouble(std::numeric_limits<double>::infinity()), 0x7ff0000000000000);
^
ACKs for top commit:
theStack:
ACK 4e44f5bac4
Tree-SHA512: 8d9e5245676c61207ceacdf78c78a78ccc9fd2a2551d4d8df023513795591334aa2f5e1f4a2a8ed2bfeb381f1e226b6ba84c07e0de29a1f3f00da71f3a257bc1
5b4703c6a7 guix: Test security-check sanity before performing them (Carl Dong)
6cf3345297 scripts: adjust test-symbol-check for guix release environment (fanquake)
1946b5f77c scripts: more robustly test macOS symbol checks (fanquake)
a8127b34bc build: Use and test PE binutils with --reloc-section (Carl Dong)
678348db51 guix: Patch binutils to add security-related disable flags (Carl Dong)
9fdc8afe11 devtools: Improve *-check.py tool detection (Carl Dong)
bda62eab38 ci: skip running the Linux test-security-check target for now (fanquake)
d6ef3543ae lint: Run mypy with --show-error-codes (Carl Dong)
Pull request description:
This is #20980 rebased (to include the Boost Process fix), and with an additional commit (892d6897f1e613084aa0517a660eab2412308e6e) to fix running the `test-security-check` target for the macOS build. It should pass inside Guix, as well as when cross-compiling on Ubuntu, or building natively on macOS.
Note that the `test-security-check` may output some warnings (similar too):
```bash
ld: warning: passed two min versions (10.14, 11.4) for platform macOS. Using 11.4.
ld: warning: passed two min versions (10.14, 11.4) for platform macOS. Using 11.4.
ld: warning: passed two min versions (10.14, 10.14) for platform macOS. Using 10.14.
```
but those can be ignored, and come about due to us passing `-platform_version` when `-mmacosx-version-min` is already part of `CC`.
Guix builds:
```bash
71ed0c7a13a4726300779ffc87f7d271086a2744c36896fe6dc51fe3dc33df2e guix-build-5b4703c6a70d/output/aarch64-linux-gnu/SHA256SUMS.part
9273980a17052c8ec45b77579781c14ab5d189fa25aa29907d5115513dd302b1 guix-build-5b4703c6a70d/output/aarch64-linux-gnu/bitcoin-5b4703c6a70d-aarch64-linux-gnu-debug.tar.gz
9c042179af43c8896eb95a34294df15d4910308dcdba40b2010cd36e192938b8 guix-build-5b4703c6a70d/output/aarch64-linux-gnu/bitcoin-5b4703c6a70d-aarch64-linux-gnu.tar.gz
1ceddecac113f50a952ba6a201cdcdb722e3dc804e663f219bfac8268ce42bf0 guix-build-5b4703c6a70d/output/arm-linux-gnueabihf/SHA256SUMS.part
759597c4e925e75db4a2381c06cda9b9f4e4674c23436148676b31c9be05c7aa guix-build-5b4703c6a70d/output/arm-linux-gnueabihf/bitcoin-5b4703c6a70d-arm-linux-gnueabihf-debug.tar.gz
34e3b6beabaf8c95d7c2ca0d2c3ac4411766694ef43e00bd9783badbbaf045a7 guix-build-5b4703c6a70d/output/arm-linux-gnueabihf/bitcoin-5b4703c6a70d-arm-linux-gnueabihf.tar.gz
e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855 guix-build-5b4703c6a70d/output/dist-archive/SKIPATTEST.TAG
3664f6ceee7898caa374281fd877a7597fe491fa2e9f0c174c28d889d60b559c guix-build-5b4703c6a70d/output/dist-archive/bitcoin-5b4703c6a70d.tar.gz
d6bc35ba0750c1440bb32831b8c12cddee62f6dce10fec2650897444c2bf4748 guix-build-5b4703c6a70d/output/powerpc64-linux-gnu/SHA256SUMS.part
a836edf6474ba0c16c19bb217549bac7936c1b44306ed512df58f607ee5568f2 guix-build-5b4703c6a70d/output/powerpc64-linux-gnu/bitcoin-5b4703c6a70d-powerpc64-linux-gnu-debug.tar.gz
7cc91c6805d5069ca3bd1771e77d95f83eb184b137198cbf84d1d11d0a5c5afe guix-build-5b4703c6a70d/output/powerpc64-linux-gnu/bitcoin-5b4703c6a70d-powerpc64-linux-gnu.tar.gz
93b4cb7b83c4975120ad5de5a92f050f5760a2a3f2c37c204c647f5a581c924a guix-build-5b4703c6a70d/output/powerpc64le-linux-gnu/SHA256SUMS.part
2266e2c5d0dafa28c6c057ccfc1c439baeab1d714d8c3f64a83015d2827116d2 guix-build-5b4703c6a70d/output/powerpc64le-linux-gnu/bitcoin-5b4703c6a70d-powerpc64le-linux-gnu-debug.tar.gz
85f41f42c319b83d049d6fd2e2278c07b40a1e28a2eac596427822c0eef9dc3f guix-build-5b4703c6a70d/output/powerpc64le-linux-gnu/bitcoin-5b4703c6a70d-powerpc64le-linux-gnu.tar.gz
1499ca9119926083d8c3714ca10d8d4c8d864cbeee8848fd8445b7a1d081222d guix-build-5b4703c6a70d/output/riscv64-linux-gnu/SHA256SUMS.part
1995fc1a2e45c49d4b0718aff5dcdac931917e8ae9e762fd23f1126abcecc248 guix-build-5b4703c6a70d/output/riscv64-linux-gnu/bitcoin-5b4703c6a70d-riscv64-linux-gnu-debug.tar.gz
266889eb58429a470f0fd7bb123f2ae09b0aef86c47b0390938b3634a8f748a9 guix-build-5b4703c6a70d/output/riscv64-linux-gnu/bitcoin-5b4703c6a70d-riscv64-linux-gnu.tar.gz
cdc3a0dcf80b110443dac5ddf8bc951001a776a651c898c5ea49bb2d487bfe29 guix-build-5b4703c6a70d/output/x86_64-apple-darwin18/SHA256SUMS.part
8538d1eab96c97866b24546c453d95822f24cf9c6638b42ba523eb7aa441cb26 guix-build-5b4703c6a70d/output/x86_64-apple-darwin18/bitcoin-5b4703c6a70d-osx-unsigned.dmg
d1b73133f1da68586b07292a8425f7f851e93f599c016376f23728c041cf39cc guix-build-5b4703c6a70d/output/x86_64-apple-darwin18/bitcoin-5b4703c6a70d-osx-unsigned.tar.gz
5ad94c5f8a5f29405955ff3ab35d137de1acc04398d6c8298fb187b57a6e316a guix-build-5b4703c6a70d/output/x86_64-apple-darwin18/bitcoin-5b4703c6a70d-osx64.tar.gz
8c6d7b3f847faa7b4d16ceecf228f26f146ea982615c1d7a00c57f9230a0c484 guix-build-5b4703c6a70d/output/x86_64-linux-gnu/SHA256SUMS.part
d0a8c99750319ad8046cfa132a54e5c13a08351f94439ae9af0f8e5486c2c2ea guix-build-5b4703c6a70d/output/x86_64-linux-gnu/bitcoin-5b4703c6a70d-x86_64-linux-gnu-debug.tar.gz
d816bb26dd4b0e309f2f576b1cccc6d78743fb2f357daad2da09bb1177330971 guix-build-5b4703c6a70d/output/x86_64-linux-gnu/bitcoin-5b4703c6a70d-x86_64-linux-gnu.tar.gz
65caaa7f648c7eab1eb82c3331a2ca25b8cd4fe41439de55604501e02571de55 guix-build-5b4703c6a70d/output/x86_64-w64-mingw32/SHA256SUMS.part
5bf6f7328cbceb0db22a2d7babb07b60cb6dcc19a6db84a1698589b7f5173a06 guix-build-5b4703c6a70d/output/x86_64-w64-mingw32/bitcoin-5b4703c6a70d-win-unsigned.tar.gz
7aabcb56115decef78d3797840b6e49dbc9b202d56f892490e92616fb06fec9e guix-build-5b4703c6a70d/output/x86_64-w64-mingw32/bitcoin-5b4703c6a70d-win64-debug.zip
2f369694648ff9dc5ca1261a1e5874b1c7408ccf2802f9caef56c1334e8a5b7c guix-build-5b4703c6a70d/output/x86_64-w64-mingw32/bitcoin-5b4703c6a70d-win64-setup-unsigned.exe
1c1f92513c4aad38419ff49a7b80bf10e6b1eca01ee8c5e3b2acd1768cf1e3d5 guix-build-5b4703c6a70d/output/x86_64-w64-mingw32/bitcoin-5b4703c6a70d-win64.zip
```
ACKs for top commit:
hebasto:
Approach ACK 5b4703c6a7.
Tree-SHA512: 2cd92a245ea64ef7176cf402a1fa5348a9421c30a4d30d01c950c48f6dcc15cf22ce69ffe1657be97e5fccc14bd933d64683c4439b695528ce3dc34d72dda927
00b875ba94 addrman: remove invalid addresses when unserializing (Vasil Dimov)
bdb62096f0 fuzz: reduce possible networks check (Vasil Dimov)
a164cd3ba6 net: simplify CNetAddr::IsRoutable() (Vasil Dimov)
Pull request description:
* Simplify some code, now that we know `CNetAddr::IsRFC4193()` and `CNetAddr::IsTor()` cannot be `true` at the same time.
* Drop Tor v2 addresses when loading addrman from `peers.dat` - they would have been loaded as dummy-all-zeros IPv6 addresses and linger in addrman, wasting space.
ACKs for top commit:
sipa:
ACK 00b875ba94. Reviewed the code, and tested with -DDEBUG_ADDRMAN (unit tests + mainnet run with peers.dat that contained v2 onions).
laanwj:
Code review and lightly tested ACK 00b875ba94
jonatack:
ACK 00b875ba94 reviewed, debug-built with -DEBUG_ADDRMAN rebased to current master, restarted node on mainnet/signet/testnet and verified that on each chain -addrinfo shows no change in address counts (as expected). Added some sanity check asserts, rebuilt/re-ran test. Checked that the new test fails on master with "test/addrman_tests.cpp(824): error: in "addrman_tests/remove_invalid": check addrman.size() == 2 has failed [4 != 2]"
jarolrod:
ACK 00b875ba94
Tree-SHA512: 6ed8e6745134b1b94fffaba28482de909ea39483b46b7f57bda61cdbae7a51251d15cb674de3631772fbeabe153d77a19269f96e62a89102a2d5c01e48f0ba06
in ProtectEvictionCandidatesByRatio().
With this change, `if (n.count == 0) continue;` will be true
if a network had candidates protected in the first iterations
and has no candidates remaining to be protected in later iterations.
Co-authored-by: Jon Atack <jon@atack.com>
for the usual case when some of the protected networks
don't have eviction candidates, to reduce the number
of iterations in ProtectEvictionCandidatesByRatio().
Picks up an idea in ef411cd2 that I had dropped.
in ProtectEvictionCandidatesByRatio().
Thank you to Vasil Dimov, whose suggestions during a post-merge
discussion about PR 21261 reminded me that I had done this in
earlier versions of the PR, e.g. commits like ef411cd2.
Co-authored-by: Vasil Dimov <vd@FreeBSD.org>
The semantic of `-bind` is to restrict the binding only to some address.
If not specified, then the user does not care and we bind to `0.0.0.0`.
If specified then we should honor the restriction and bind only to the
specified address.
Before this change, if no `-bind` is given then we would bind to
`0.0.0.0:8333` and to `127.0.0.1:8334` (incoming Tor) which is ok -
the user does not care to restrict the binding.
However, if only `-bind=addr:port=onion` is given (without ordinary
`-bind=`) then we would bind to `addr:port` _and_ to `0.0.0.0:8333` in
addition.
Change the above to not do the additional bind: if only
`-bind=addr:port=onion` is given (without ordinary `-bind=`) then bind
to `addr:port` (only) and consider incoming connections to that as Tor
and do not advertise it. I.e. a Tor-only node.
This is important to make sure that we're not testing tools different
from the one we're building with.
Introduce determine_wellknown_cmd, which encapsulates how we
should handle well-known tools specification (IFS splitting, env
override, etc.).
797b3ed909 script: remove gitian reference from symbol-check.py (fanquake)
15fc9a0299 guix: add additional documentation to patches (fanquake)
4516e5ec92 lint: exclude Guix patches from spell-checking (fanquake)
de6ca41a52 guix: no-longer pass --enable-glibc-back-compat to Guix (fanquake)
84dd81fb5b build: remove glibc backcompat requirement for Linux symbol checks (fanquake)
Pull request description:
Now that our Guix toolchains are based on glibc 2.24 and 2.27 (RISCV), we don't need to use the `--enable-glibc-back-compat` option to produce binaries that don't use any symbols from glibc 2.17 and 2.27 or later.
This also adds additional documentation to some Guix patches (pointed out in #22365) and removes Guix patches from the spelling linter, because that isn't our spelling.
Symbol usage: https://gist.github.com/fanquake/d15604fc580718444c5aa4b3c3c75fdc.
Guix Builds:
```bash
bash-5.1# find guix-build-$(git rev-parse --short=12 HEAD)/output/ -type f -print0 | env LC_ALL=C sort -z | xargs -r0 sha256sum
ed54e6a6cf4fab328557c0c72eb08c73f2a58c6c70959544cf4b1882e75ea69e guix-build-797b3ed90900/output/aarch64-linux-gnu/SHA256SUMS.part
83bd9dadc59f89f848d143fa4fc3964f16fe0b4bdf35e5093b577ff2c4bd1f43 guix-build-797b3ed90900/output/aarch64-linux-gnu/bitcoin-797b3ed90900-aarch64-linux-gnu-debug.tar.gz
94cb8c35281f12dec6ea5b390b66cad5e27ac8c45a30c42c8d38c438695d54c0 guix-build-797b3ed90900/output/aarch64-linux-gnu/bitcoin-797b3ed90900-aarch64-linux-gnu.tar.gz
7318b63d65c0aa52d2446de8e1f40658d2e47ab8fb0268820c3b7585d140fb23 guix-build-797b3ed90900/output/arm-linux-gnueabihf/SHA256SUMS.part
95e1ffb372964b73f539653ca703b70cf0c018801a9c4c0ffc46a0b63539253c guix-build-797b3ed90900/output/arm-linux-gnueabihf/bitcoin-797b3ed90900-arm-linux-gnueabihf-debug.tar.gz
039d3842e6499626cf955ae0a7590dd6b3d0935cdc217c98aaf9d156b0ebd3b4 guix-build-797b3ed90900/output/arm-linux-gnueabihf/bitcoin-797b3ed90900-arm-linux-gnueabihf.tar.gz
e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855 guix-build-797b3ed90900/output/dist-archive/SKIPATTEST.TAG
2c4e7b6e7aff63ba811e5bf59362d16866c3a358f8844fba8739a61192870622 guix-build-797b3ed90900/output/dist-archive/bitcoin-797b3ed90900.tar.gz
955029b949c368eabd517dd33040d2f01e2ac6a55e7b4f9107907a7c6e0c6060 guix-build-797b3ed90900/output/powerpc64-linux-gnu/SHA256SUMS.part
fd6d6b137f8efedf58a879d11205b1d4649e1f97d7f91e193239ef206fcc285d guix-build-797b3ed90900/output/powerpc64-linux-gnu/bitcoin-797b3ed90900-powerpc64-linux-gnu-debug.tar.gz
51736ac8e77737999f1b5bd4c381b0016f19a8d5e40e786fe941ff04e84c11c9 guix-build-797b3ed90900/output/powerpc64-linux-gnu/bitcoin-797b3ed90900-powerpc64-linux-gnu.tar.gz
8c244c16bfa46c1efdb120e1d91fdd14d3f14eefee8d7e1fbb0a9b4664a5c315 guix-build-797b3ed90900/output/powerpc64le-linux-gnu/SHA256SUMS.part
704ee593251a1b1c65a5bebeef93b23f266af4e8cbf8ae556150c3b2e8f06a6c guix-build-797b3ed90900/output/powerpc64le-linux-gnu/bitcoin-797b3ed90900-powerpc64le-linux-gnu-debug.tar.gz
0ec06ae7d344de20d61e3965d8b383747ef20b0e9d93a3165733ea23bdf2ead8 guix-build-797b3ed90900/output/powerpc64le-linux-gnu/bitcoin-797b3ed90900-powerpc64le-linux-gnu.tar.gz
2dd6c6ecc67b0ea40ca9c43f92efca81ccd054b8db8c197ad84ad9674d510a25 guix-build-797b3ed90900/output/riscv64-linux-gnu/SHA256SUMS.part
5ebb27a855a677f7a188d83995be6b2a3ea8606be152abb7fc7832713fb0677a guix-build-797b3ed90900/output/riscv64-linux-gnu/bitcoin-797b3ed90900-riscv64-linux-gnu-debug.tar.gz
bdaf1783f5e1861597afa37c1880364e118d9a7a7af8017302d82202791019f6 guix-build-797b3ed90900/output/riscv64-linux-gnu/bitcoin-797b3ed90900-riscv64-linux-gnu.tar.gz
726c9092b60ac2e7d7e14b2c24467fcf276a6f89170a871ddab9dce6ac230699 guix-build-797b3ed90900/output/x86_64-apple-darwin18/SHA256SUMS.part
2af4d709b44952654f3c08c86593bf2ccc9a44ed422783a1b95b8a199a894db2 guix-build-797b3ed90900/output/x86_64-apple-darwin18/bitcoin-797b3ed90900-osx-unsigned.dmg
fd49ba445aa6cf3d8c47019a05e9e5740cb0f53349344dd80671297127f49f1a guix-build-797b3ed90900/output/x86_64-apple-darwin18/bitcoin-797b3ed90900-osx-unsigned.tar.gz
3f51cbf8cf18420d4be70e656aa993675cf5e828a255c2030047ae2e059ed5b7 guix-build-797b3ed90900/output/x86_64-apple-darwin18/bitcoin-797b3ed90900-osx64.tar.gz
afd1edee1447bb88d81e972abfae4c4e065b5b1827769f033cff9472084c7c1b guix-build-797b3ed90900/output/x86_64-linux-gnu/SHA256SUMS.part
ec468ef886d25e685f4f7a18b4f7d497dedf757495e0d5beb72c23cc32ab69b5 guix-build-797b3ed90900/output/x86_64-linux-gnu/bitcoin-797b3ed90900-x86_64-linux-gnu-debug.tar.gz
1934d7294f0c9e083d38a3f68d4a61cd679defa79ce0a89f77386978692b9b18 guix-build-797b3ed90900/output/x86_64-linux-gnu/bitcoin-797b3ed90900-x86_64-linux-gnu.tar.gz
94c11c328a628052eb6f50e9816aa768f87ea7acfbbbafdab60f6928da766811 guix-build-797b3ed90900/output/x86_64-w64-mingw32/SHA256SUMS.part
fd371922ba93d81bd4a2b711d617af6756f9f0494db6d83aa0e5f491a24168ef guix-build-797b3ed90900/output/x86_64-w64-mingw32/bitcoin-797b3ed90900-win-unsigned.tar.gz
4e4ad976bc029bbbf9596ad8493accaaba8b0d5c598dd342f8da330609bbdf21 guix-build-797b3ed90900/output/x86_64-w64-mingw32/bitcoin-797b3ed90900-win64-debug.zip
3a89a16b9101e9a17d98efb9234b5bdd264c0bba2c6326511017730e1a08311f guix-build-797b3ed90900/output/x86_64-w64-mingw32/bitcoin-797b3ed90900-win64-setup-unsigned.exe
e285ab737e3c843fd3f1c26c2f053e421a3c39b33995747ce48281884d3f28d1 guix-build-797b3ed90900/output/x86_64-w64-mingw32/bitcoin-797b3ed90900-win64.zip
```
ACKs for top commit:
sipa:
utACK 797b3ed909
hebasto:
ACK 797b3ed909
Tree-SHA512: 3a569702d8832c155c5ce8d2f6d823f7f12603885576078bc5192bc9038a48261ecb541800f79d1e9bc86d71fa640265c5b8b89df9d8bb680b3bb05d9d78a666
986bf78d7e qt: Emit dataChanged signal to dynamically re-sort Peers table (Hennadii Stepanov)
Pull request description:
[By default](https://doc.qt.io/qt-5/qsortfilterproxymodel.html#details), the `PeerTableSortProxy`
> dynamically re-sorts ... data whenever the original model changes.
That is not the case on master (8cdf91735f) as in ecbd911538 (#164) no signals are emitted to notify about model changes.
This PR uses a dedicated [`dataChanged`](https://doc.qt.io/qt-5/qabstractitemmodel.html#dataChanged) signal.
Fixes#367.
An alternative to #374.
ACKs for top commit:
jarolrod:
ACK 986bf78d7e
Tree-SHA512: dcb92c2f9a2c632880429e9528007db426d2ad938c64dfa1f1538c03e4b62620df52ad7daf33b582976c67b472ff76bc0dae707049f4bbbd4941232cee9ce3d4
cd46c11577 qt: Draw "eye" sign at the beginning of watch-only addresses (Hennadii Stepanov)
9ea1da6fc9 qt: Do not extend recent transaction width to address/label string (Hennadii Stepanov)
Pull request description:
This PR guaranties that the "eye" sign won't be hidden for very long addresses/labels.
No longer need to extend `TransactionOverviewWidget` widget width to make "eye" signs shown:
Fixes https://github.com/bitcoin-core/gui/issues/373
ACKs for top commit:
jarolrod:
ACK cd46c11577
Tree-SHA512: 0602b5bb65d53c5b18e86260750006bba03adbae181917b5a2b7f89b17290bd1f57b4f80adaba32f42cc6fb468598a888b12c0b6b09005d2f2c07bd4d1ad334a
* Assert when a type is missing
* Add missing WitnessV1Taproot
* Limit WitnessUnknown to version [2, 16], to avoid abiguity
* Limit WitnessUnknown to size [2, 40], to avoid invalid sizes
e48826ad87 tests: remove ComputeBlockVersion shortcut from versionbits tests (Anthony Towns)
c5f36725e8 [refactor] Move ComputeBlockVersion into VersionBitsCache (Anthony Towns)
4a69b4dbe0 [move-only] Move ComputeBlockVersion from validation to versionbits (Anthony Towns)
0cfd6c6a8f [refactor] versionbits: make VersionBitsCache a full class (Anthony Towns)
8ee3e0bed5 [refactor] rpc/blockchain.cpp: SoftForkPushBack (Anthony Towns)
92f48f360d deploymentinfo: Add DeploymentName() (Anthony Towns)
ea68b3a572 [move-only] Rename versionbitsinfo to deploymentinfo (Anthony Towns)
c64b2c6a0f scripted-diff: rename versionbitscache (Anthony Towns)
de55304f6e [refactor] Add versionbits deployments to deploymentstatus.h (Anthony Towns)
2b0d291da8 [refactor] Add deploymentstatus.h (Anthony Towns)
eccd736f3d versionbits: Use dedicated lock instead of cs_main (Anthony Towns)
36a4ba0aaa versionbits: correct doxygen comments (Anthony Towns)
Pull request description:
Introduces helper functions to make it easy to bury future deployments, along the lines of the suggestion from [11398](https://github.com/bitcoin/bitcoin/pull/11398#issuecomment-335599326) "I would prefer it if a buried deployment wouldn't require all code paths that check the BIP9 status to require changing".
This provides three functions: `DeploymentEnabled()` which tests if a deployment can ever be active, `DeploymentActiveAt()` which checks if a deployment should be enforced in the given block, and `DeploymentActiveAfter()` which checks if a deployment should be enforced in the block following the given block, and overloads all three to work both with buried deployments and versionbits deployments.
This adds a dedicated lock for the versionbits cache, which is acquired internally by the versionbits functions, rather than relying on `cs_main`. It also moves moves versionbitscache into deploymentstatus to avoid a circular dependency with validation.
ACKs for top commit:
jnewbery:
ACK e48826ad87
gruve-p:
ACK e48826ad87
MarcoFalke:
re-ACK e48826ad87🥈
Tree-SHA512: c846ba64436d36f8180046ad551d8b0d9e20509b9bc185aa2639055fc28803dd8ec2d6771ab337e80da0b40009ad959590d5772f84a0bf6199b65190d4155bed
67669ab425 build: Fix Boost Process compatibility with mingw-w64 compiler (Hennadii Stepanov)
Pull request description:
On master (9c3751a0c9) the cross build for Win64 is broken if configured with `--enable-external-signer`:
```
...
CXX crypto/libbitcoin_crypto_base_a-chacha_poly_aead.o
In file included from /home/hebasto/GitHub/bitcoin/depends/x86_64-w64-mingw32/include/boost/process/detail/windows/handles.hpp:11,
from /home/hebasto/GitHub/bitcoin/depends/x86_64-w64-mingw32/include/boost/process/detail/used_handles.hpp:17,
from /home/hebasto/GitHub/bitcoin/depends/x86_64-w64-mingw32/include/boost/process/detail/windows/async_in.hpp:20,
from /home/hebasto/GitHub/bitcoin/depends/x86_64-w64-mingw32/include/boost/process/async.hpp:49,
from /home/hebasto/GitHub/bitcoin/depends/x86_64-w64-mingw32/include/boost/process.hpp:23,
from util/system.cpp:9:
/home/hebasto/GitHub/bitcoin/depends/x86_64-w64-mingw32/include/boost/process/detail/windows/handle_workaround.hpp:208:51: error: expected ‘)’ before ‘*’ token
208 | typedef ::boost::winapi::NTSTATUS_ (__kernel_entry *nt_system_query_information_p )(
| ~ ^~
| )
/home/hebasto/GitHub/bitcoin/depends/x86_64-w64-mingw32/include/boost/process/detail/windows/handle_workaround.hpp:223:51: error: expected ‘)’ before ‘*’ token
223 | typedef ::boost::winapi::NTSTATUS_ (__kernel_entry *nt_query_object_p )(
| ~ ^~
| )
/home/hebasto/GitHub/bitcoin/depends/x86_64-w64-mingw32/include/boost/process/detail/windows/handle_workaround.hpp: In function ‘boost::winapi::NTSTATUS_ boost::process::detail::windows::workaround::nt_system_query_information(boost::process::detail::windows::workaround::SYSTEM_INFORMATION_CLASS_, void*, boost::winapi::ULONG_, boost::winapi::PULONG_)’:
/home/hebasto/GitHub/bitcoin/depends/x86_64-w64-mingw32/include/boost/process/detail/windows/handle_workaround.hpp:239:12: error: ‘nt_system_query_information_p’ does not name a type; did you mean ‘nt_system_query_information’?
239 | static nt_system_query_information_p f = reinterpret_cast<nt_system_query_information_p>(::boost::winapi::get_proc_address(h, "NtQuerySystemInformation"));
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| nt_system_query_information
In file included from /home/hebasto/GitHub/bitcoin/depends/x86_64-w64-mingw32/include/boost/process/detail/windows/handles.hpp:11,
from /home/hebasto/GitHub/bitcoin/depends/x86_64-w64-mingw32/include/boost/process/detail/used_handles.hpp:17,
from /home/hebasto/GitHub/bitcoin/depends/x86_64-w64-mingw32/include/boost/process/detail/windows/async_in.hpp:20,
from /home/hebasto/GitHub/bitcoin/depends/x86_64-w64-mingw32/include/boost/process/async.hpp:49,
from /home/hebasto/GitHub/bitcoin/depends/x86_64-w64-mingw32/include/boost/process.hpp:23,
from util/system.cpp:9:
/home/hebasto/GitHub/bitcoin/depends/x86_64-w64-mingw32/include/boost/process/detail/windows/handle_workaround.hpp:241:14: error: ‘f’ was not declared in this scope
241 | return (*f)(SystemInformationClass, SystemInformation, SystemInformationLength, ReturnLength);
| ^
/home/hebasto/GitHub/bitcoin/depends/x86_64-w64-mingw32/include/boost/process/detail/windows/handle_workaround.hpp: In function ‘boost::winapi::BOOL_ boost::process::detail::windows::workaround::nt_query_object(boost::winapi::HANDLE_, boost::process::detail::windows::workaround::OBJECT_INFORMATION_CLASS_, void*, boost::winapi::ULONG_, boost::winapi::PULONG_)’:
/home/hebasto/GitHub/bitcoin/depends/x86_64-w64-mingw32/include/boost/process/detail/windows/handle_workaround.hpp:253:12: error: ‘nt_query_object_p’ does not name a type; did you mean ‘nt_query_object’?
253 | static nt_query_object_p f = reinterpret_cast<nt_query_object_p>(::boost::winapi::get_proc_address(h, "NtQueryObject"));
| ^~~~~~~~~~~~~~~~~
| nt_query_object
/home/hebasto/GitHub/bitcoin/depends/x86_64-w64-mingw32/include/boost/process/detail/windows/handle_workaround.hpp:255:14: error: ‘f’ was not declared in this scope
255 | return (*f)(Handle, ObjectInformationClass, ObjectInformation, ObjectInformationLength, ReturnLength);
| ^
make[2]: *** [Makefile:9906: util/libbitcoin_util_a-system.o] Error 1
make[2]: *** Waiting for unfinished jobs....
CXX crypto/libbitcoin_crypto_base_a-chacha20.o
make[2]: Leaving directory '/home/hebasto/GitHub/bitcoin/src'
make[1]: *** [Makefile:16141: all-recursive] Error 1
make[1]: Leaving directory '/home/hebasto/GitHub/bitcoin/src'
make: *** [Makefile:820: all-recursive] Error 1
```
The upstream bug: https://github.com/boostorg/process/issues/96
Also see: https://stackoverflow.com/a/59338759https://github.com/bitcoin/bitcoin/pull/22348#issuecomment-871061160:
> [This commit](7fc41b2815), containing the `__kernel_entry` [SAL annotations](https://docs.microsoft.com/en-us/cpp/code-quality/using-sal-annotations-to-reduce-c-cpp-code-defects?view=msvc-160) was included in Boost Process as part of the `1.71.0` release, which broke support for compiling with mingw-w64 because it doesn't define the `__kernel_entry` SAL annotation (but it does define some others, i.e see [`sal.h`](https://github.com/mirror/mingw-w64/blob/master/mingw-w64-headers/include/sal.h)).
>
> A [commit was made](d7a721ee0d) to remove the annotations, however, it hasn't made it into either of the two Boost releases that have happened since (1.75.0 & 1.76.0). Meaning that this is currently needed for all versions of Boost process from 1.71.0 onwards.
ACKs for top commit:
fanquake:
ACK 67669ab425 - thanks for updating this.
Tree-SHA512: 5931ca1fb77ce38c042cf5a7556add024ea2386c208bf26c792a8ca4a771d97fac9802c32fa8aa2e3de1ad35f3362d8c066f0a83ee675859d226c602fd0bcf93
6084d2caed wallet: do not spam about non-existent spk managers (S3RK)
Pull request description:
Avoid spam in logs during `loadwallet`, `listdescriptors` and probably other commands as well.
**`loadwallet` Before:**
```
2021-06-24T06:31:45Z init message: Loading wallet…
2021-06-24T06:31:45Z [desc] Wallet File Version = 169900
2021-06-24T06:31:45Z [desc] Keys: 0 plaintext, 0 encrypted, 0 w/ metadata, 0 total. Unknown wallet records: 0
2021-06-24T06:31:45Z [desc] External scriptPubKey Manager for output type 0 does not exist
2021-06-24T06:31:45Z [desc] External scriptPubKey Manager for output type 1 does not exist
2021-06-24T06:31:45Z [desc] External scriptPubKey Manager for output type 2 does not exist
2021-06-24T06:31:45Z [desc] Internal scriptPubKey Manager for output type 0 does not exist
2021-06-24T06:31:45Z [desc] Internal scriptPubKey Manager for output type 1 does not exist
2021-06-24T06:31:45Z [desc] Internal scriptPubKey Manager for output type 2 does not exist
2021-06-24T06:31:45Z [desc] Wallet completed loading in 197ms
2021-06-24T06:31:45Z [desc] External scriptPubKey Manager for output type 0 does not exist
2021-06-24T06:31:45Z [desc] External scriptPubKey Manager for output type 1 does not exist
2021-06-24T06:31:45Z [desc] External scriptPubKey Manager for output type 2 does not exist
2021-06-24T06:31:45Z [desc] Internal scriptPubKey Manager for output type 0 does not exist
2021-06-24T06:31:45Z [desc] Internal scriptPubKey Manager for output type 1 does not exist
2021-06-24T06:31:45Z [desc] Internal scriptPubKey Manager for output type 2 does not exist
2021-06-24T06:31:45Z [desc] External scriptPubKey Manager for output type 0 does not exist
2021-06-24T06:31:45Z [desc] External scriptPubKey Manager for output type 1 does not exist
2021-06-24T06:31:45Z [desc] External scriptPubKey Manager for output type 2 does not exist
2021-06-24T06:31:45Z [desc] Internal scriptPubKey Manager for output type 0 does not exist
2021-06-24T06:31:45Z [desc] Internal scriptPubKey Manager for output type 1 does not exist
2021-06-24T06:31:45Z [desc] Internal scriptPubKey Manager for output type 2 does not exist
2021-06-24T06:31:45Z [desc] setKeyPool.size() = 0
2021-06-24T06:31:45Z [desc] mapWallet.size() = 0
2021-06-24T06:31:45Z [desc] m_address_book.size() = 0
2021-06-24T06:31:45Z [desc] External scriptPubKey Manager for output type 0 does not exist
2021-06-24T06:31:45Z [desc] External scriptPubKey Manager for output type 1 does not exist
2021-06-24T06:31:45Z [desc] External scriptPubKey Manager for output type 2 does not exist
2021-06-24T06:31:45Z [desc] Internal scriptPubKey Manager for output type 0 does not exist
2021-06-24T06:31:45Z [desc] Internal scriptPubKey Manager for output type 1 does not exist
2021-06-24T06:31:45Z [desc] Internal scriptPubKey Manager for output type 2 does not exist
{
"name": "desc",
"warning": ""
}
```
**After:**
```
2021-06-24T06:26:58Z init message: Loading wallet…
2021-06-24T06:26:58Z [desc] Wallet File Version = 169900
2021-06-24T06:26:58Z [desc] Keys: 0 plaintext, 0 encrypted, 0 w/ metadata, 0 total. Unknown wallet records: 0
2021-06-24T06:26:58Z [desc] Wallet completed loading in 158ms
2021-06-24T06:26:58Z [desc] setKeyPool.size() = 0
2021-06-24T06:26:58Z [desc] mapWallet.size() = 0
2021-06-24T06:26:58Z [desc] m_address_book.size() = 0
{
"name": "desc",
"warning": ""
}
```
ACKs for top commit:
achow101:
ACK 6084d2caed
Tree-SHA512: c7d7345c3182a575db088fd731b7f6e428c42e4f3f2e10d5adb50bf74a2defe88768e65ebb91a08590be48cf766a5697e36fafa73f68ffe45e76a60600f072e2
b945a31afa wallet: erase spkmans rather than setting to nullptr (Andrew Chow)
Pull request description:
In many places in ScriptPubKeyMan managing code, we assume that the ScriptPubKeyMan being retrieved actually exists and is not a nullptr. Thus removing a ScriptPubKeyMan requires erasing the object from the map rather than setting it to a nullptr.
This fixes a segmentation fault that can be reached with `test/functional/wallet_descriptors.py --descriptors`
ACKs for top commit:
S3RK:
ACK b945a31
Tree-SHA512: 344a4cf9b1c168428750c751dcd24c52032506f20c81977fe93c4b5307ea209de72bb62a9c5284820f225b03acdc9573fceb734833d29b82f49d5a799ddcaea7
2f23ad2c40 qt: allow prompt icon to be colorized (Jarol Rodriguez)
Pull request description:
Opening the console on macOS, while in dark mode, the console prompt icon will not be colorized white like other icons. This applies the `platformStyle` to the icon so that It can be colorized white.
While here, refactor the `promptIcon` widget from a `QPushButton` to `QLabel`; which is more appropriate, per [Qt Docs](https://doc.qt.io/qt-5/qlabel.html#details):
> QLabel is used for displaying text or an image. No user interaction functionality is provided.
| Master | PR |
| ----------- | ----------- |
|  |  |
ACKs for top commit:
hebasto:
ACK 2f23ad2c40
Tree-SHA512: 21f8b1610e4820c9064bbd08608b5467e5b9499e2a3b149ff223e37b60e7d560497255c733eafa5434628a84b9f7b7c91d8b0f34b02be2f9ceb3ab21a4d555a8
9d5bf6bf01 GUI: Always call parent changeEvent handler (Luke Dashjr)
c901d4d8ce GUI: Enable palette change adaptation on all platforms (Luke Dashjr)
Pull request description:
The changes to support macOS "Dark Mode" are valid for any platform, and should work so long as Qt implements the PaletteChange event. (Worst case, we're no worse off with trying.)
Additionally, we shouldn't block the parent classes from implementing event handlers. Who knows what side effects that could have.
ACKs for top commit:
hebasto:
ACK 9d5bf6bf01, tested on Linux Mint 20.1 (Qt 5.12.8) with the [`qt5ct`](https://packages.ubuntu.com/focal/qt5ct) package installed.
kristapsk:
ACK 9d5bf6bf01. Tested on Gentoo Linux with Xfce4 and Qt 5.15.2, does not break anything on my computer.
Tree-SHA512: dce2fff0ff129eda208132390a37424ff9607539287dbdbfdfd659ed9c4ea0472541e987489a04fd935e391dc006a35bfc9cfa9bcff33602b7dbd29b81c51626
In many places in ScriptPubKeyMan managing code, we assume that the
ScriptPubKeyMan being retrieved actually exists and is not a nullptr.
Thus removing a ScriptPubKeyMan requires erasing the object from the
map rather than setting it to a nullptr.
181181019c refactor: remove m_internal from DescriptorSPKman (S3RK)
Pull request description:
Rationale: improve consistency between `CWallet` and `DescriptorScriptPubKeyMan`; simplify `ScriptPubKeyMan` interface.
Descriptor in itself is neither internal or external. It's responsibility of a wallet to assign and manage descriptors for a specific purpose. Duplicating information about internalness of a descriptor could lead to inconsistencies and unexpected behaviour (for example misreporting keypool size).
ACKs for top commit:
instagibbs:
reACK 181181019c
achow101:
reACK 181181019c
Tree-SHA512: d5613b7f6795b290bfa0fd8cb0536de1714d0cf72cba402266bd06d550758ebad690b54fc0a336a1c7414b5814aa4a37c90a6ae89926474a97d30956d7e034ff
3efaf83c75 wallet: deactivate descriptor (S3RK)
6737d9655b test: wallet importdescriptors update existing (S3RK)
586f1d53d6 wallet: maintain SPK consistency on internal flag change (S3RK)
f1b7db1474 wallet: don't mute exceptions in importdescriptors (S3RK)
bf68ebc1cd wallet: allow to import same descriptor twice (S3RK)
Pull request description:
Rationale: allow updating existing descriptors with `importdescriptors` command.
Currently if you run same `importdescriptors` command twice with a descriptor containing private key you will get very confusing error — `Missing required fields`. What happens is that Wallet tries to write imported private key to the disk, but it exists already so we get `DB_KEYEXIST (-30995)` from BerkelyDB. Please note, that we set `DB_NOOVERWRITE` (I guess not to lose some keys accidentally). The exception is caught in `catch (...)` in rpcdump.cpp with a generic error.
With this PR if a descriptor is already present than we will update its activeness, internalness, label, range and next_index.
For the range only expansion is allowed (range start can only decrease, range end increase).
ACKs for top commit:
achow101:
re-ACK 3efaf83c75
meshcollider:
Code review ACK 3efaf83c75
jonatack:
Light ACK 3efaf83c75 per `git range-diff a000cb0 5d96704 3efaf83` and as a sanity check, re-debug-built on debian with gcc 10.2.1 and clang 11, ran wallet_importdescriptors.py
Tree-SHA512: 122c4b621d64ec8a3b625f3aed9f01a2b5cbaf2029ad0325b5ff38d67fff5cd35324335fabe2dd5169548b01b267c81be6ae0f5c834342f3d5f6eeed515c4843
e6cf0ed92d wallet, rpc: listdescriptors does not need unlocked (Andrew Chow)
3280704886 Pass in DescriptorCache to ToNormalizedString (Andrew Chow)
7a26ff10c2 Change DescriptorImpl::ToStringHelper to use an enum (Andrew Chow)
75530c93a8 Remove priv option for ToNormalizedString (Andrew Chow)
74fede3b8b wallet: Upgrade existing descriptor caches (Andrew Chow)
432ba9e543 wallet: Store last hardened xpub cache (Andrew Chow)
d87b544b83 descriptors: Cache last hardened xpub (Andrew Chow)
cacc391098 Move DescriptorCache writing to WalletBatch (Andrew Chow)
0b4c8ef75c Refactor Cache merging and writing (Andrew Chow)
976b53b085 Revert "Cache parent xpub inside of BIP32PubkeyProvider" (Andrew Chow)
Pull request description:
Currently fetching a normalized descriptor requires the wallet to be unlocked as it needs the private keys to derive the last hardened xpub. This is not very user friendly as normalized descriptors shouldn't require and don't involve the private keys except for derivation. We solve this problem by caching the last hardened xpub (which has to be derived at some point when generating the address pool).
However the last hardened xpub was not already being cached. We only cached the immediate parent xpub and derived child keys. For example, with a descriptor derivation path of `/84'/0'/0'/0/*`, the parent xpub that is cached is `m/84'/0'/0'/0`, and the child keys of `m/84'/0'/0'/0/i` (note that child keys would not be cached in this case). This parent xpub is not suitable for the normalized descriptor form as we want the key at `m/84'/0'/0'`. So this PR adds another field to `DescriptorCache` to cache the last hardened xpub so that we can use them for normalized descriptors.
Since `DescriptorCache` is changing, existing descriptor wallets need to be upgraded to use this new cache. The upgrade will occur in the background either at loading time (if the wallet is not encrypted) or at unlocking time in the same manner that `UpgradeKeyMetadata` operates. It will use a new wallet flag `WALLET_FLAG_LAST_HARDENED_XPUB_CACHED` to indicate whether the descriptor wallet has the last hardened xpub cache.
Lastly `listdescriptors` will not require the wallet to be locked and `getaddressinfo`'s `parent_desc` will always be output (assuming the upgrade has occurred).
ACKs for top commit:
fjahr:
tACK e6cf0ed92d
S3RK:
reACK e6cf0ed
jonatack:
Semi ACK e6cf0ed92d reviewed, debug-built and ran unit tests and some of the descriptor functional tests at each commit. I'm not very familiar with this code and it could be clearer to the uninitiated IMHO, so I'm not confident enough to give a full ACK. Various minor suggestions follow, most of them for readability, feel free to pick and choose.
meshcollider:
Code review + functional test run ACK e6cf0ed92d
Tree-SHA512: ac27aade8644525cd65bfcaf27ff32afb974085b1451faf4ff68c6671a690bd6a41d4f39a33cbf461ae0fbe85995c0a4c08dbd36171da1c1d2a1d00053ad298d
f9e37f33ce doc: IsFinalTx comment about nSequence & OP_CLTV (Yuval Kogman)
Pull request description:
It's somewhat surprising that a transaction's `nLockTime` field is ignored
when all `nSequence` fields are final, so this change aims to clarify this
behavior and cross reference relevant details of `OP_CHECKLOCKTIMEVERIFY`.
ACKs for top commit:
MarcoFalke:
ACK f9e37f33ce
Tree-SHA512: 88460dacbe4b8115fb1948715f09b21d4f34ba1da9e88d52f0b774a969f845e9eddc5940e7fee66eacdd3062dc40d6d44c3f282b0e5144411fd47eb2320b44f5
Descriptor in itself is neither internal or external.
It's responsibility of a wallet to assign and manage descriptors
for a specific purpose. Duplicating such information could lead to
inconsistencies and unexpected behaviour.
Rename BIP9SoftForkPushBack and BuriedSoftForkPushBack to SoftForkPushBack
and have the compiler figure out which one to use based on the deployment
type. Avoids the need to update the file when burying a deployment.
Adds support for versionbits deployments to DeploymentEnabled,
DeploymentActiveAfter and DeploymentActiveAt. Also moves versionbitscache
from validation to deploymentstatus.
Provides DeploymentEnabled, DeploymentActiveAt, and DeploymentActiveAfter
helpers for checking the status of buried deployments. Can be overloaded
so the same syntax works for non-buried deployments, allowing future
soft forks to be changed from signalled to buried deployments without
having to touch the implementation code.
Replaces IsWitnessEnabled and IsScriptWitnessEnabled.
fa92e60f38 refactor: Make httpserver work queue a unique_ptr (MarcoFalke)
Pull request description:
This simplifies the code a bit because `if (p) { delete p; p = nullptr; }` can be replaced by a call to the `reset()` member.
ACKs for top commit:
promag:
Core review ACK fa92e60f38.
jonatack:
ACK fa92e60f38 code review, debug build clean, ran test/functional/interface*.py tests locally as a sanity check
hebasto:
ACK fa92e60f38, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: 6b122162317dd4ad6889341745c7ac1903a3ee510f6548f46dc356308442a6eff13eb8dc604c38ba18783e7a66d2b836d641a8594ff980a010c12c97f3856684
8888cf45f5 Remove unused wallet pointer from NotifyAddressBookChanged (MarcoFalke)
faf3640303 Remove unused wallet pointer from NotifyTransactionChanged signal (MarcoFalke)
Pull request description:
The signals are members of the wallet, so passing the pointer would be redundant even if it was used.
Also, fix `with` -> `without`, which was forgotten in commit ca4cf5cff6.
ACKs for top commit:
jonatack:
Code review ACK 8888cf45f5 also verified with/without lock cs_wallet status for each of the two functions and debian clang 11 debug build clean
promag:
Code review ACK 8888cf45f5.
theStack:
Code review ACK 8888cf45f5
Tree-SHA512: e3b80931ce9bcb05213619f5435ac7c21d3c7848643950a70db610902bd1803c92bb75e501d46b0e519bc576901f160e088e8882c4f1adce892a80df565f897b
fa0d9211ef refactor: Remove chainparams arg from CChainState member functions (MarcoFalke)
fa38947125 refactor: Remove ::Params() global from inside CChainState member functions (MarcoFalke)
Pull request description:
The `::Params()` global is verbose and confusing. Also it makes tests a bit harder to write because they'd have to mock a global.
Fix all issues by simply using a member variable that points to the right params.
(Can be reviewed with `--word-diff-regex=.`)
ACKs for top commit:
jnewbery:
ACK fa0d9211ef
kiminuo:
utACK fa0d9211
theStack:
ACK fa0d9211ef🍉
Tree-SHA512: 44676b19c9ed471ccb536331d3029bad192d7d50f394fd7b8527ec431452aeec8c4494164b9cf8e16e0123c4463b16be864366c6b599370032c17262625a0356
fa9ebedec3 Reject invalid coin height and output index when loading assumeutxo (MarcoFalke)
Pull request description:
It should be impossible to have a coin at a height higher than the height of the snapshot block, so reject those early to avoid integer wraparounds and hash collisions later on.
Same for the outpoint index.
Both issues were found by fuzzing:
* The height issue by OSS-Fuzz: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=34793
* The outpoint issue by my fuzz server: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=34793#c2
ACKs for top commit:
practicalswift:
cr ACK fa9ebedec3: patch looks correct
jamesob:
crACK fa9ebedec3
theStack:
Code review ACK fa9ebedec3
benthecarman:
crACK fa9ebedec3
Tree-SHA512: dae7caee4b3862b23ebdf2acb7edec4baf75b0dbf1409b370b1a73aa6b632b317ebfac596dcbaf4edfb1301b513f45465ea75328962460f35e2af0d7e547c9ac
.PHONY does not take patterns (such as print-%) as prerequisites.
Have print-% depend on FORCE and mark FORCE as phony.
$ # on master
$ make print-host
host=x86_64-pc-linux-gnu
$ touch print-host
$ make print-host
make: 'print-host' is up to date.
$
$ git co mark_print_as_phony
Switched to branch 'mark_print_as_phony'
$ make print-host
host=x86_64-pc-linux-gnu
$ touch FORCE
$ make print-host
host=x86_64-pc-linux-gnu
Instead of having a large blob of cache merging code in TopUp, refactor
this into DescriptorCache so that it can merge and provide a diff
(another DescriptorCache containing just the items that were added).
Then TopUp can just write everything that was in the diff.
fa34cb8024 cli: Avoid truncating -rpcwaittimeout (MarcoFalke)
Pull request description:
`seconds` is not enough precision to "exactly" store a timestamp n seconds into the future. Improve the precision by using `microseconds`. Fixes#22325
Also, use chrono literals.
ACKs for top commit:
jonatack:
ACK fa34cb8024 review, debug-built, tested
theStack:
Tested ACK fa34cb8024
Tree-SHA512: 7158da8545f9998a82bcc8636e04564efdb1e1be43b4288298c151b4df29ad47a2760259eefadd4a01db92ea18a1e017f3febc1cd8c69a4b28c86180229d8c90
754f134a50 wallet: Add error message to GetReservedDestination (Andrew Chow)
87a0e7a3b7 Disallow bech32m addresses for legacy wallet things (Andrew Chow)
6dbe4d1072 Use BECH32M for tr() desc, WitV1Taproot, and WitUnknown CTxDests (Andrew Chow)
699dfcd8ad Opportunistically use bech32m change addresses if available (Andrew Chow)
0262536c34 Add OutputType::BECH32M (Andrew Chow)
177c15d2f7 Limit LegacyScriptPubKeyMan address types (Andrew Chow)
Pull request description:
Currently bech32m addresses are classfied as bech32. Because bech32m is incompatible with bech32, we need to define a new `OutputType` for it so that it can be handled correctly. This PR adds `OutputType::BECH32M`, updates all of the relevant `OutputType` classifications, and handle requests for bech32m addresses. There is now a `bech32m` address type string that can be used.
* `tr()` descriptors now report their output type as `OutputType::BECH32M`. `WtinessV1Taproot` and `WitnessUnknown` are also classified as `OutputType::BECH32M`.
* Bech32m addresses are completely disabled for legacy wallets. They cannot be imported (explicitly disallowed in `importaddress` and `importmulti`), will not be created when getting all destinations for a pubkey, and will not be added with `addmultisigaddress`. Additional protections have been added to `LegacyScriptPubKeyMan` to disallow attempting to retrieve bech32m addresses.
* Since Taproot multisigs are not implemented yet, `createmultisig` will also disallow the bech32m address type.
* As Taproot is not yet active, `DescriptorScriptPubKeyMan` cannot and will not create a `tr()` descriptor. Protections have been added to make sure this cannot occur.
* The change address type detection algorithm has been updated to return `bech32m` when there is a segwit v1+ output script and the wallet has a bech32m `ScriptPubKeyMan`, falling back to bech32 if one is not available.
ACKs for top commit:
laanwj:
re-review ACK 754f134a50
Sjors:
re-utACK 754f134: only change is switching to `bech32m` in two `wallet_taproot.py` test cases.
fjahr:
re-ACK 754f134a50
jonatack:
ACK 754f134a50
Tree-SHA512: 6ea90867d3631d0d438e2b08ce6ed930f37d01323224661e8e38f183ea5ee2ab65b5891394a3612c7382a1aff907b457616c6725665a10c320174017b998ca9f
BIP324 mentions K1 is used for the associated data and K2 is used for
the payload. The code does the opposite. This is not a security problem
but will be a problem across implementations based on the HKDF key
derivations.
7ad414f4bf doc: add comment about CCoinsViewDBCursor constructor (James O'Beirne)
0f8a5a4dd5 move-only(ish): don't expose CCoinsViewDBCursor (James O'Beirne)
615c1adfb0 refactor: wrap CCoinsViewCursor in unique_ptr (James O'Beirne)
Pull request description:
I tripped over this one for a few hours at the beginning of the week, so I've sort of got a personal vendetta against `CCoinsView::Cursor()` returning a raw pointer.
Specifically in the case of CCoinsViewDB, if a raw cursor is allocated and not freed, a cryptic leveldb assertion failure occurs on CCoinsViewDB destruction (`Assertion 'dummy_versions_.next_ == &dummy_versions_' failed.`).
This is a pretty simple change.
Related to: https://github.com/bitcoin/bitcoin/issues/21766
See also: https://github.com/google/leveldb/issues/142#issuecomment-414418135
ACKs for top commit:
MarcoFalke:
review ACK 7ad414f4bf🔎
jonatack:
re-ACK 7ad414f4bf modulo suggestion
ryanofsky:
Code review ACK 7ad414f4bf. Two new commits look good and thanks for clarifying constructor comment
Tree-SHA512: 6471d03e2de674d84b1ea0d31e25f433d52aa1aa4996f7b4aab1bd02b6bc340b15e64cc8ea07bbefefa3b5da35384ca5400cc230434e787c30931b8574c672f9
d637a9b397 Taproot descriptor inference (Pieter Wuille)
c7388e5ada Report address as solvable based on inferred descriptor (Pieter Wuille)
29e5dd1a5b consensus refactor: extract ComputeTapleafHash, ComputeTaprootMerkleRoot (Pieter Wuille)
Pull request description:
Includes:
* First commit from #21365, adding TaprootSpendData in SigningProvider
* A refactor to expose ComputeTapleafHash and ComputeTaprootMerkleRoot from script/interpreter
* A tiny change to make `getaddressinfo` report tr() descriptors as solvable (so that inferred descriptors are shown), despite not having signing code for them.
* Logic to infer the script tree back from TaprootSpendData, and then use that to infer descriptors.
ACKs for top commit:
achow101:
re-ACK d637a9b397
Sjors:
re-utACK d637a9b
meshcollider:
Code review ACK d637a9b397
Tree-SHA512: 5ab9b95da662382d8549004be4a1297a577d7caca6b068f875c7c9343723931d03fa9cbf133de11f83b74e4851490ce820fb80413c77b9e8495a5f812e505d86
bb719a08db style: remove () from assert in rpc_setban.py (Vasil Dimov)
24b10ebda3 doc: fix grammar in doc/files.md (Vasil Dimov)
dd4e957dcd test: ensure banlist can be read from disk after restart (Vasil Dimov)
d197977ae2 banman: save the banlist in a JSON format on disk (Vasil Dimov)
Pull request description:
Save the banlist in `banlist.json` instead of `banlist.dat`.
This makes it possible to store Tor v3 entries in the banlist on disk
(and any other addresses that cannot be serialized in addrv1 format).
Only read `banlist.dat` if it exists and `banlist.json` does not exist (first start after an upgrade).
Supersedes https://github.com/bitcoin/bitcoin/pull/20904
Resolves https://github.com/bitcoin/bitcoin/issues/19748
ACKs for top commit:
jonatack:
Code review re-ACK bb719a08db per `git range-diff 6a67366 4b52c72 bb719a0`
achow101:
Code Review ACK bb719a08db
Tree-SHA512: fc135c3a1fe20bcf5d008ce6bea251b4135e56c78bf8f750b4bd8144c095b81ffe165133cdc7e4715875eec7e7c4e13ad9f5d2450b21102af063d7c8abf716b6
Adds an error output parameter to all GetReservedDestination functions
so that callers can get the actual reason that a change address could
not be fetched. This more closely matches GetNewDestination. This allows
for more granular error messages, such as one that indicates that
bech32m addresses cannot be generated yet.
We don't want the legacy wallet to ever have bech32m addresses so don't
allow importing them. This includes addmultisigaddress as that is a
legacy wallet only RPC
Additionally, bech32m multisigs are not available yet, so disallow them
in createmultisig.
If a transaction as a segwit output, use a bech32m change address if
they are available. If not, fallback to bech32. If bech32 change
addresses are unavailable, fallback to the default address type.
Bech32m addresses need their own OutputType
We are not ready to create DescriptorScriptPubKeyMans which produce
bech32m addresses. So don't allow generating them.
b9e76f1bf0 rpc: Add test for -rpcwaittimeout (Christian Decker)
f76cb10d7d rpc: Prefix rpcwaittimeout error with details on its nature (Christian Decker)
c490e17ef6 doc: Add release notes for the `-rpcwaittimeout` cli parameter (Christian Decker)
a7fcc8eb59 rpc: Add a `-rpcwaittimeout` parameter to limit time spent waiting (Christian Decker)
Pull request description:
Adds a new numeric `-rpcwaittimeout` that can be used to limit the
time we spend waiting on the RPC server to appear. This is used by
downstream projects to provide a bit of slack when `bitcoind`s RPC
interface is not available right away.
This makes the `-rpcwait` argument more useful, since we can now limit
how long we'll ultimately wait, before potentially giving up and reporting
an error to the caller. It was discussed in the context of the BTCPayServer
wanting to have c-lightning wait for the RPC interface to become available
but still have the option of giving up eventually ([4355]).
I checked with laanwj whether this is already possible ([comment]), and
whether this would be a welcome change. Initially I intended to repurpose
the (optional) argument to `-rpcwait`, however I decided against it since it
would potentially break existing configurations, using things like `rpcwait=1`,
or `rpcwait=true` (the former would have an unintended short timeout, when
old behavior was to wait indefinitely).
~Due to its simplicity I didn't implement a test for it yet, but if that's desired I
can provide one.~ Test was added during reviews.
[4355]: https://github.com/ElementsProject/lightning/issues/4355
[comment]: https://github.com/ElementsProject/lightning/issues/4355#issuecomment-768288261
ACKs for top commit:
laanwj:
Code review ACK b9e76f1bf0
promag:
ACK b9e76f1bf0.
Tree-SHA512: 3cd6728038ec7ca7c35c2e7ccb213bfbe963f99a49bb48bbc1e511c4dd23d9957c04f9af1f8ec57120e47b26eaf580b46817b099d5fc5083c98da7aa92db8638
Save the banlist in `banlist.json` instead of `banlist.dat`.
This makes it possible to store Tor v3 entries in the banlist on disk
(and any other addresses that cannot be serialized in addrv1 format).
Only read `banlist.dat` if it exists and `banlist.json` does not
exist (first start after an upgrade).
Supersedes https://github.com/bitcoin/bitcoin/pull/20904
Resolves https://github.com/bitcoin/bitcoin/issues/19748
4e353cb618 http: Release work queue after event base finish (João Barbosa)
Pull request description:
This fixes a race between `http_request_cb` and `StopHTTPServer` where
the work queue is used after release.
Fixes#18856.
ACKs for top commit:
fjahr:
Code review ACK 4e353cb618
achow101:
ACK 4e353cb618
LarryRuane:
ACK 4e353cb618
hebasto:
ACK 4e353cb618, tested (rebased on top of master 9313c4e6aa) on Linux Mint 20.1 (x86_64) using MarcoFalke's [patch](https://github.com/bitcoin/bitcoin/pull/19033#issuecomment-640106647), including different `-rpcthreads`/`-rpcworkqueue` cases. The bug is fixed. The code is correct.
Tree-SHA512: 185d2a9744d0d5134d782bf321ac9958ba17b11a5b3d70b4897c8243e6b146dfd3f23c57aef8e10ae9484374120b64389c1949a9cf0a21dccc47ffc934c20930
30aee2dfe6 tests: Add test for compact block HB selection (Pieter Wuille)
6efbcec4de Protect last outbound HB compact block peer (Suhas Daftuar)
Pull request description:
If all our high-bandwidth compact block serving peers (BIP 152) stall block
download, then we can be denied a block for (potentially) a long time. As
inbound connections are much more likely to be adversarial than outbound
connections, mitigate this risk by never removing our last outbound HB peer if
it would be replaced by an inbound.
ACKs for top commit:
achow101:
ACK 30aee2dfe6
ariard:
Code ACK 30aee2dfe
jonatack:
ACK 30aee2dfe6
Tree-SHA512: 5c6c9326e3667b97e0864c371ae2174d2be9054dad479f4366127b9cd3ac60ffa01ec9707b16ef29cac122db6916cf56fd9985733390017134ace483278921d5
458a345b05 Add support for SIGHASH_DEFAULT in RPCs, and make it default (Pieter Wuille)
c0f0c8eccb tests: check spending of P2TR (Pieter Wuille)
a2380127e9 Basic Taproot signing logic in script/sign.cpp (Pieter Wuille)
49487bc3b6 Make GetInputUTXO safer: verify non-witness UTXO match (Pieter Wuille)
fd3f6890f3 Construct and use PrecomputedTransactionData in PSBT signing (Pieter Wuille)
5cb6502ac5 Construct and use PrecomputedTransactionData in SignTransaction (Pieter Wuille)
5d2e22437b Don't nuke witness data when signing fails (Pieter Wuille)
ce9353164b Permit full precomputation in PrecomputedTransactionData (Pieter Wuille)
e841fb503d Add precomputed txdata support to MutableTransactionSignatureCreator (Pieter Wuille)
a91d532338 Add CKey::SignSchnorr function for BIP 340/341 signing (Pieter Wuille)
e77a2839b5 Use HandleMissingData also in CheckSchnorrSignature (Pieter Wuille)
dbb0ce9fbf Add TaprootSpendData data structure, equivalent to script map for P2[W]SH (Pieter Wuille)
Pull request description:
Builds on top of #22051, adding signing support after derivation support.
Nothing is changed in descriptor features. Signing works for key path and script path spending, through the normal sending functions, and PSBT-based RPCs. However, PSBT usability is rather low as no extensions have been defined to convey Taproot-specific information, so all script information must be known to the signing wallet.
ACKs for top commit:
achow101:
re-ACK 458a345b05
fjahr:
Code review ACK 458a345b05
Sjors:
ACK 458a345b05
Tree-SHA512: 30ed212cf7754763a4a81624ebc084c51727b8322711ac0b390369213c1a891d367ed8b123882ac08c99595320c11ec57ee42304ff22a69afdc3d1a0d55cc711
9550dffa0c fuzz: Assert roundtrip equality for `CPubKey` (Sebastian Falbesoner)
Pull request description:
This PR is a (quite late) follow-up to #19237 (https://github.com/bitcoin/bitcoin/pull/19237#issuecomment-642203251). Looking at `CPubKey::Serialize` and `CPubKey::Unserialize` I can't think of a scenario where the roundtrip (serialization/deserialization) equality wouldn't hold.
ACKs for top commit:
jamesob:
crACK 9550dffa0c pending CI
Tree-SHA512: 640fb9e777d249769b22ee52c0b15a68ff0645b16c986e1c0bce9742155d14f1be601e591833e1dc8dcffebf271966c6b861b90888a44aae1feae2e0248e2c55
f8866e8c32 Add roundtrip fuzz tests for CAddress serialization (Pieter Wuille)
e2f0548b52 Use addrv2 serialization in anchors.dat (Pieter Wuille)
8cd8f37dfe Introduce well-defined CAddress disk serialization (Pieter Wuille)
Pull request description:
Alternative to #20509.
This makes the `CAddress` disk serialization format well defined, and uses it to enable addrv2 support in anchors.dat (in a way that's compatible with older software). The new format is:
- The first 4 bytes store a format version number. Its low 19 bits are ignored (as those historically stored the `CLIENT_VERSION`), but its high 13 bits specify the actual serialization:
- 0x00000000: LE64 encoding for `nServices`, V1 encoding for `CService` (like pre-BIP155 network serialization).
- 0x20000000: CompactSize encoding for `nServices`, V2 encoding for `CService` (like BIP155 network serialization).
- Any other value triggers an unsupported format error on deserialization, and can be used for future format changes.
- The `ADDRV2_FORMAT` flag in the stream's version does not determine the actual serialization format; it only sets whether or not V2 encoding is permitted.
ACKs for top commit:
achow101:
ACK f8866e8c32
laanwj:
Code review ACK f8866e8c32
vasild:
ACK f8866e8c32
jonatack:
ACK f8866e8c32 tested rebased to master and built/run/restarted with DEBUG_ADDRMAN, peers.dat and anchors ser/deser seems fine
hebasto:
ACK f8866e8c32, tested on Linux Mint 20.1 (x86_64).
Tree-SHA512: 3898f8a8c51783a46dd0aae03fa10060521f5dd6e79315fe95ba807689e78f202388ffa28c40bf156c6f7b1fc2ce806b155dcbe56027df73d039a55331723796
faf1af58f8 fuzz: Add Temporary debug assert for oss-fuzz issue (MarcoFalke)
Pull request description:
oss-fuzz is acting weird, so add an earlier assert to help troubleshooting
ACKs for top commit:
practicalswift:
cr ACK faf1af58f8
Tree-SHA512: 85830d7d47cf6b4edfe91a07bd5aa8f7110db0bade8df93868cf276ed04d5dd17e671f769e6a0fb5092012b86aa82bb411fb171411f15746981104ce634c88c1
2f5bdcbc31 gui: misc external signer fixes and translation hints (Sjors Provoost)
d672404466 refactor: make ExternalSigner NetworkArg() and m_chain private (Sjors Provoost)
4455145e26 refactor: reduce #ifdef ENABLE_EXTERNAL_SIGNER usage (Sjors Provoost)
5be90c907e build: enable external signer by default (Sjors Provoost)
7d9453041b refactor: clean up external_signer.h includes (Sjors Provoost)
fc0eca31b3 fuzz: fix fuzz binary linking order (Sjors Provoost)
Pull request description:
This follows the introduction of GUI support in https://github.com/bitcoin-core/gui/pull/4
I don't think we should expect GUI users to self compile. This also enables external signer support by default for RPC users.
In addition this PR reduces the number of `#ifdef ENABLE_EXTERNAL_SIGNER`, which also fixes#21919. When compiled with `--disable-external-signer` such wallets can't be created in RPC or GUI, but they can be loaded. Attempting any action that calls HWI will trigger an error.
Side-note: this PR may or may not (currently) break CI for the GUI repository, as explained here: https://github.com/bitcoin-core/gui/pull/4#issuecomment-769859001
ACKs for top commit:
achow101:
ACK 2f5bdcbc31
hebasto:
re-ACK 2f5bdcbc31
Tree-SHA512: 1b71c5a8bea2be077ee9fa33a01130c957a0cf90951d4b7b04d3d0ef826bb77e474c3963abddfef2e2c1ea99d9c72cd2302d1eb9b5fcb7ba0bd2a625f006aa05
We encountered a linking error when attempting to include external_signer_scriptpubkeyman.cpp when configured with --disable-external-signer.
Everywhere else we have LIBBITCOIN_WALLET, it is always before LIBBITCOIN_COMMON. But if you go up to where FUZZ_SUITE_LD_COMMON is first set, you see that we will end up having LIBBITCOIN_COMMON set before LIBBITCOIN_WALLET which means that the linker will have problems linking things common things that the wallet uses. Because the order is correct for the other targets, we only see a linker error for test/fuzz/fuzz.
In this diff, LIBTEST_UTIL and LIBTEST_FUZZ are moved to the top because they include LIBBITCOIN_SERVER and LIBBITCOIN_COMMON. LIBBITCOIN_SERVER always needs to be the first item in the linker order since it has the most dependencies.
The makefiles for making the fuzz and test binaries should be revisited so that the linking order is made consistent with the rest of the code and to avoid other linker order issues that may crop up in the future.
Co-Authored-By: Andrew Chow <achow101-github@achow101.com>
79c02c88b3 Randomize message processing peer order (Pieter Wuille)
Pull request description:
Right now, the message handling loop iterates the list of nodes always in the same order: the order they were connected in (see the `vNodes` vector). For some parts of the net processing logic, this order matters. Transaction requests are assigned explicitly to peers since #19988, but many other parts of processing work on a "first-served-by-loop-first" basis, such as block downloading. If peers can predict this ordering, it may be exploited to cause delays.
As there isn't anything particularly optimal about the current ordering, just make it unpredictable by randomizing.
Reported by Crypt-iQ.
ACKs for top commit:
jnewbery:
ACK 79c02c88b3
Crypt-iQ:
ACK 79c02c88b3
sdaftuar:
utACK 79c02c88b3
achow101:
Code Review ACK 79c02c88b3
jamesob:
crACK 79c02c88b3
jonatack:
ACK 79c02c88b3
vasild:
ACK 79c02c88b3
theStack:
ACK 79c02c88b3
Tree-SHA512: 9a87c4dcad47c2d61b76c4f37f59674876b78f33f45943089bf159902a23e12de7a5feae1a73b17cbc3f2e37c980ecf0f7fd86af9e6fa3a68099537a3c82c106
e4c916a0ea Bugfix: GUI: Use a different shortcut for "1 d&ay" banning, due to conflict with "&Disconnect" (Luke Dashjr)
94e7cdd7e0 GUI: Add keyboard shortcuts for other context menus (Luke Dashjr)
02b5263cd4 GUI: Restore keyboard shortcuts for context menu entries (Luke Dashjr)
Pull request description:
Various keyboard shortcuts were lost in #263; this restores them, and also adds new ones for other context menus.
Note that with a context menu open, simply the shortcut by itself (no Alt) is used.
ACKs for top commit:
jarolrod:
Code Review ACK e4c916a
hebasto:
ACK e4c916a0ea, tested on Linux Mint 20.1 (Qt 5.12.8).
Tree-SHA512: 949461acf7aac592bc48a1c5abad41b167365830e0cedb3aa11b6a87bd347e16126830ea87936f9c9efc4b7df5b09d3833fae784964d6d119ed45703cfba2ffd
The unit test is single threaded, so there's no need to hold the mutex
between Good() and Attempt().
This change avoids recursive locking in the CAddrMan::Attempt function.
Co-authored-by: John Newbery <john@johnnewbery.com>
1b1088d52f test: add combined I2P/onion/localhost eviction protection tests (Jon Atack)
7c2284eda2 test: add tests for inbound eviction protection of I2P peers (Jon Atack)
ce02dd1ef1 p2p: extend inbound eviction protection by network to I2P peers (Jon Atack)
70bbc62711 test: add combined onion/localhost eviction protection coverage (Jon Atack)
045cb40192 p2p: remove unused m_is_onion member from NodeEvictionCandidate struct (Jon Atack)
310fab4928 p2p: remove unused CompareLocalHostTimeConnected() (Jon Atack)
9e889e8a5c p2p: remove unused CompareOnionTimeConnected() (Jon Atack)
787d46bb2a p2p: update ProtectEvictionCandidatesByRatio() doxygen docs (Jon Atack)
1e15acf478 p2p: make ProtectEvictionCandidatesByRatio() fully ratio-based (Jon Atack)
3f8105c4d2 test: remove combined onion/localhost eviction protection tests (Jon Atack)
38a81a8e20 p2p: add CompareNodeNetworkTime() comparator struct (Jon Atack)
4ee7aec47e p2p: add m_network to NodeEvictionCandidate struct (Jon Atack)
7321e6f2fe p2p, refactor: rename vEvictionCandidates to eviction_candidates (Jon Atack)
ec590f1d91 p2p, refactor: improve constness in ProtectEvictionCandidatesByRatio() (Jon Atack)
4a19f501ab test: add ALL_NETWORKS to test utilities (Jon Atack)
519e76bb64 test: speed up and simplify peer_eviction_test (Jon Atack)
1cde800523 p2p, refactor: rm redundant erase_size calculation in SelectNodeToEvict() (Jon Atack)
Pull request description:
Continuing the work in #20197 and #20685, this pull updates and abstracts our inbound eviction protection to make it fully ratio-based and easily extensible to peers connected via high-latency privacy networks that we newly support, like I2P and perhaps others soon, as these peers are disadvantaged by the latency criteria of our eviction logic.
It then adds eviction protection for peers connected over I2P. As described in https://github.com/bitcoin/bitcoin/pull/20685#issuecomment-767486499, we've observed over the past few months that I2P peers have a min ping latency similar to or greater than that of onion peers.
The algorithm is a basically a multi-pass knapsack:
- Count the number of eviction candidates in each of the disadvantaged
privacy networks.
- Sort the networks from lower to higher candidate counts, so that
a network with fewer candidates will have the first opportunity
for any unused slots remaining from the previous iteration. In
the case of a tie in candidate counts, priority is given by array
member order from first to last, guesstimated to favor more unusual
networks.
- Iterate through the networks in this order. On each iteration,
allocate each network an equal number of protected slots targeting
a total number of candidates to protect, provided any slots remain
in the knapsack.
- Protect the candidates in that network having the longest uptime,
if any in that network are present.
- Continue iterating as long as we have non-allocated slots
remaining and candidates available to protect.
The goal of this logic is to favorise the diversity of our peer connections.
The individual commit messages describe each change in more detail.
Special thank you to Vasil Dimov for the excellent review feedback and the algorithm improvement that made this change much better than it would have been otherwise. Thanks also to Antoine Riard, whose review feedback nudged this change to protect disadvantaged networks having fewer, rather than more, eviction candidates.
ACKs for top commit:
laanwj:
Code review re-ACK 1b1088d52f
vasild:
ACK 1b1088d52f
Tree-SHA512: 722f790ff11f2969c79e45a5e0e938d94df78df8687e77002f32e3ef5c72a9ac10ebf8c7a9eb7f71882c97ab0e67b2778191effdb747d9ca54d7c23c2ed19a90
This commit extends our inbound eviction protection to I2P peers to
favorise the diversity of peer connections, as peers connected
through the I2P network are otherwise disadvantaged by our eviction
criteria for their higher latency (higher min ping times) relative
to IPv4 and IPv6 peers, as well as relative to Tor onion peers.
The `networks` array is order-dependent in the case of a tie in
candidate counts between networks (earlier array members receive
priority in the case of a tie).
Therefore, we place I2P candidates before localhost and onion ones
in terms of opportunity to recover unused remaining protected slots
from the previous iteration, guesstimating that most nodes allowing
both onion and I2P inbounds will have more onion peers, followed by
localhost, then I2P, as I2P support is only being added in the
upcoming v22.0 release.
with a more abstract framework to allow easily extending inbound
eviction protection to peers connected through new higher-latency
networks that are disadvantaged by our inbound eviction criteria,
such as I2P and perhaps other BIP155 networks in the future like
CJDNS. This is a change in behavior.
The algorithm is a basically a multi-pass knapsack:
- Count the number of eviction candidates in each of the disadvantaged
privacy networks.
- Sort the networks from lower to higher candidate counts, so that
a network with fewer candidates will have the first opportunity
for any unused slots remaining from the previous iteration. In
the case of a tie in candidate counts, priority is given by array
member order from first to last, guesstimated to favor more unusual
networks.
- Iterate through the networks in this order. On each iteration,
allocate each network an equal number of protected slots targeting
a total number of candidates to protect, provided any slots remain
in the knapsack.
- Protect the candidates in that network having the longest uptime,
if any in that network are present.
- Continue iterating as long as we have non-allocated slots
remaining and candidates available to protect.
Localhost peers are treated as a network like Tor or I2P by aliasing
them to an unused Network enumerator: Network::NET_MAX.
The goal is to favorise diversity of our inbound connections.
Credit to Vasil Dimov for improving the algorithm from single-pass
to multi-pass to better allocate unused protection slots.
Co-authored-by: Vasil Dimov <vd@FreeBSD.org>
as we are about the change the behavior sufficiently that when we
have multiple disadvantaged networks and a small number of peers
under test, the number of protected peers per network can be different.
This speeds up the test significantly, which helps when
running it repeatedly.
Suggest reviewing the diff with:
colorMoved = dimmed-zebra
colorMovedWs = allow-indentation-change
At verification time, the to be precomputed data can be inferred from
the transaction itself. For signing, the necessary witnesses don't
exist yet, so just permit precomputing everything in that case.
