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
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
