376e01b382679b49d5c8464c869d14eca23ab4b9 doc: add databases/py-sqlite3 to FreeBSD test suite deps (fanquake)
Pull request description:
Adds missing documentation. See also https://cirrus-ci.com/task/5639240319500288.
ACKs for top commit:
john-moffett:
ACK 376e01b382679b49d5c8464c869d14eca23ab4b9
Tree-SHA512: a7b8d0bae00c3538934d23eae207b7927a64e748eb228ac6c5754aee0e9b6796c0f39066c7d81cc009bf442c8b1a0c31739adde87d1ebc3445aed54dda47c9ce
6d0ab07e817725369df699791b9c5b2fae204990 refactor: use convenience fn to auto parse non-string parameters (stickies-v)
Pull request description:
Minimizes code duplication and improves function naming by having a single (overloaded) convenience function `ParseIfNonString` that both checks if the parameter is a non-string parameter and automatically parses the value if so.
ACKs for top commit:
aureleoules:
ACK 6d0ab07e817725369df699791b9c5b2fae204990
Tree-SHA512: 8cbf68a17cfbdce1e31a19916f447a2965c139fdef00c19e32a9b679f4a4015dfe69ceea0bbe1723711e1c5033ea8d4005d1f4485dfbeea22226140f8cbe8aa3
c6119f478876f245ca5c65dd05da4cdc5be0e91f tests: Use unique port for ZMQ tests (Andrew Chow)
Pull request description:
The ZMQ interface tests should use unique ports as we do for the p2p and rpc ports so that multiple instances of the test can be run at the same time.
Without this, the test may hang until killed, or fail.
ACKs for top commit:
MarcoFalke:
ACK c6119f478876f245ca5c65dd05da4cdc5be0e91f
Tree-SHA512: 2ca3ed2f35e5a83d7ab83740674fed362a8d146dc751156cfe100133a591347cd1ac9d164046f1744d65451a57c52cb22d3bb2161105f421f8f655c4a2512c59
affbf58a1e52a8e60c830be6a9e0347e0ff4c28e build: Move environment variables into `$(package)_config_env` (Hennadii Stepanov)
d44fcd3c976572883bbf7f386bc88e2610dc1a58 build: Make $(package)_*_env available to all $(package)_*_cmds (Hennadii Stepanov)
Pull request description:
On master (1e7564eca8a688f39c75540877ec3bdfdde766b1) the depends build system, which is based on pure GNU Make, works, but it lacks robustness, and in some corner cases it fails. For example, see bitcoin/bitcoin#22552.
Another [bug](https://github.com/bitcoin/bitcoin/issues/22719) in the depends build system has already become a problem at least two times in the past (https://github.com/bitcoin/bitcoin/pull/16883#issuecomment-683817472 and https://github.com/bitcoin/bitcoin/pull/24134). Each time the problem was solved with other means.
The initial [solution](https://github.com/bitcoin/bitcoin/pull/19882) had some discussion. Also it was discussed on the IRC meeting in #bitcoin-core-builds channel. This PR, actually, is a resurrection of it, as the bug silently struck pretty [recently](https://github.com/bitcoin/bitcoin/pull/24134).
The bug is well described in bitcoin/bitcoin#22719.
Here is another, a bit simpler description, which requires only basic shell (bash, dash etc) experience.
After creating targets by this code:1e7564eca8/depends/funcs.mk (L280) a "draft" line of recipe like `$($(1)_config_env) $(call $(1)_config_cmds, $(1))` becomes a shell command sequence `VAR1=foo VAR2=bar command1 && command2` which is supposed to be executed in a [new sub-shell](https://www.gnu.org/software/make/manual/html_node/Execution.html#Execution).
Please note that `VAR1=foo VAR2=bar` part is visible for the first `command1` only (for details see shell docs). Example:
```sh
$ VAR1="foo" VAR2="bar" echo "begin" && printenv VAR1 && printenv VAR2 && echo "end"
begin
$ echo $?
1
```
Using the `export` command is a trivial solution:
```sh
$ export VAR1="foo" VAR2="bar"; echo "begin" && printenv VAR1 && printenv VAR2 && echo "end"
begin
foo
bar
end
$ echo $?
0
```
As a [new sub-shell](https://www.gnu.org/software/make/manual/html_node/Execution.html#Execution) is invoked for each line of the recipe, there are no side effects of using `export`. It means this solution should not be considered invasive.
Fixesbitcoin/bitcoin#22719.
---
Also this PR removes no longer needed crutch from `qt.mk`.
ACKs for top commit:
fanquake:
ACK affbf58a1e52a8e60c830be6a9e0347e0ff4c28e
Tree-SHA512: 0ce2cf82870a7774bdf1592fac50857126ae47da902e349f1092d50109223be9d6a8efd5e92ec08c2ca775b17516482aabaf232378950ade36484a883acc177b
31d0067f8bccd6b090bf88bad8472bb5cb1f20a4 doc: test: update/fix TestShell example instructions (Sebastian Falbesoner)
Pull request description:
This PR tackles two issues in the TestShell documentation:
- add missing instruction for creating a wallet prior to the `getnewaddress` call (needed as there is no default wallet created anymore since v0.21)
- fix `generatetoaddress` call syntax (the scripted-diff in commit fa0b916971e5bc23ad6396831940a2899ca05402 only worked for tests using `BitcoinTestFramework`)
ACKs for top commit:
fanquake:
ACK 31d0067f8bccd6b090bf88bad8472bb5cb1f20a4 - current instructions don't work. These do.
Tree-SHA512: d2b7808a06892ad16728cb2b6d4a72b255ad711d27fe98b1de562f80444e7bb25d73296abdde4308162fe3be702864e2f7b7dbbbb000fe54c709951c09e6c730
fa3ea81c3e7d962715788ab5525958a532c51414 refactor: Add LIFETIMEBOUND / -Wdangling-gsl to Assert() (MacroFake)
Pull request description:
Currently compiles clean, but I think it may still be useful.
Can be tested by adding an `&`:
```diff
diff --git a/src/test/util_tests.cpp b/src/test/util_tests.cpp
index 5766fff92d..300c1ec60f 100644
--- a/src/test/util_tests.cpp
+++ b/src/test/util_tests.cpp
@@ -125,7 +125,7 @@ BOOST_AUTO_TEST_CASE(util_check)
// Check -Wdangling-gsl does not trigger when copying the int. (It would
// trigger on "const int&")
- const int nine{*Assert(std::optional<int>{9})};
+ const int& nine{*Assert(std::optional<int>{9})};
BOOST_CHECK_EQUAL(9, nine);
}
```
Output:
```
test/util_tests.cpp:128:29: warning: object backing the pointer will be destroyed at the end of the full-expression [-Wdangling-gsl]
const int& nine{*Assert(std::optional<int>{9})};
^~~~~~~~~~~~~~~~~~~~~
./util/check.h:75:50: note: expanded from macro 'Assert'
#define Assert(val) inline_assertion_check<true>(val, __FILE__, __LINE__, __func__, #val)
^~~
1 warning generated.
ACKs for top commit:
jonatack:
ACK fa3ea81c3e7d962715788ab5525958a532c51414
theuni:
ACK fa3ea81c3e7d962715788ab5525958a532c51414
Tree-SHA512: 17dea4d75f2ee2bf6e1b6a6f6d8f439711c777df0390574e8d8edb6ac9ee807a135341e4439050bd6a15ecc4097a1ba9a7ab15d27541ebf70a4e081fa6871877
5c9a27a46f4b07f872f850f7e918c10a33595cfd test: Use proper Boost macros instead of assertions (Hennadii Stepanov)
Pull request description:
On the master branch:
```
$ src/test/test_bitcoin -l test_suite -t banman_tests
Running 1 test case...
...
Test case banman_tests/file did not check any assertions
...
```
This PR suggests to use proper Boost [macros](https://www.boost.org/doc/libs/1_80_0/libs/test/doc/html/boost_test/utf_reference/testing_tool_ref.html).
Top commit has no ACKs.
Tree-SHA512: e0c8e5e6371acd0e0a80070fffdf1445f264c62499f8d9811822994c89735a913c18c8ed730495578400abdd93d2d500345504f2a9246401d53fb2f9f71be8c5
0dbafcee46 Merge bitcoin/bitcoin#27289: Refactor: Remove unused FlatFilePos::SetNull (fanquake)
dbe2e04d62 Merge bitcoin/bitcoin#27212: test: Make the unlikely race in p2p_invalid_messages impossible (fanquake)
6f6b718f78 Merge bitcoin/bitcoin#27236: util: fix argsman dupe key error (fanquake)
74c6e38530 Merge bitcoin/bitcoin#27205: doc: Show how less noisy clang-tidy output can be achieved (fanquake)
9e552f0293 Merge bitcoin/bitcoin#27232: Use string interpolation for default value of -listen (fanquake)
2a39b93233 Merge bitcoin/bitcoin#27226: test: Use self.wait_until over wait_until_helper (fanquake)
be2e16f33a Merge bitcoin/bitcoin#27192: util: add missing include and fix function signature (fanquake)
176a4a60d2 Merge bitcoin/bitcoin#27173: valgrind: remove libsecp256k1 suppression (fanquake)
d2fc8be331 Merge bitcoin/bitcoin#27154: doc: mention sanitizer suppressions in developer docs (glozow)
f5b4cc7e32 Merge bitcoin/bitcoin#16195: util: Use void* throughout support/lockedpool.h (Andrew Chow)
c66c0fdbf8 Merge bitcoin/bitcoin#27137: test: Raise PRNG seed log to INFO (fanquake)
bba215031b Merge bitcoin/bitcoin#25950: test: fix test abort for high timeout values (and `--timeout-factor 0`) (fanquake)
6751add2ea Merge bitcoin/bitcoin#27107: doc: remove mention of "proper signing key" (merge-script)
34c895a542 Merge bitcoin/bitcoin#26997: psbt: s/transcation/transaction/ (fanquake)
befdbeddf9 Merge bitcoin/bitcoin#27097: descriptors: fix docstring (param [in] vs [out]) (fanquake)
c98dd824b6 Merge bitcoin/bitcoin#27080: Wallet: Zero out wallet master key upon locking so it doesn't persist in memory (Andrew Chow)
Pull request description:
## Issue being fixed or feature implemented
Batch of trivial backports
## What was done?
See commits
## How Has This Been Tested?
built locally; large combined merge passed tests locally
## Breaking Changes
Should be none
## Checklist:
- [ ] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_
ACKs for top commit:
knst:
utACK 0dbafcee46
UdjinM6:
utACK 0dbafcee46
Tree-SHA512: e93a1136e848aa6c6f3d9fb0567b3e284975d35e82bbc1d9a8cd908067df4bf1257c939882abcaca6820360a4982b991f505a1165d95e1a8b52c9b181b7026b7
BACKPORT NOTE:
Missing changes in src/fs.cpp are removed in bitcoin/bitcoin#20744 which is already backported
b367745cfe19f6de3f44b3adc90fa08e36e44bb6 ci: Make Cirrus CI Windows build with --enable-werror (Hennadii Stepanov)
c713bb2b243881a771ab288340ffeb623c82d7f6 Fix Windows build with --enable-werror on Ubuntu Focal (Hennadii Stepanov)
Pull request description:
This PR makes possible to cross-compile Windows build with `--enable-werror --enable-suppress-external-warnings`.
Some problems are fixed, others are silenced.
Also `--enable-werror` is enabled for Cirrus CI Windows build (the last one on Cirrus CI without `--enable-werror`).
ACKs for top commit:
practicalswift:
cr ACK b367745cfe19f6de3f44b3adc90fa08e36e44bb6: patch looks correct
laanwj:
Code review ACK b367745cfe19f6de3f44b3adc90fa08e36e44bb6
vasild:
ACK b367745cfe19f6de3f44b3adc90fa08e36e44bb6
jarolrod:
ACK b367745cfe19f6de3f44b3adc90fa08e36e44bb6
Tree-SHA512: 64f5c99b7dad4c0efce80cd45d7074f275bd8411235dc9e0841287bdab64b812c6f8f9d632c35531d0b8210148531f53aaaac77be7699b29d2d6aaae304dbee0
62b125fd197879f112322a1f67a318d6ab22e67a qt, refactor: Fix indentation (Prateek Sancheti)
ad28b66e98c9bb3bc7af2545654842544a798601 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 62b125fd197879f112322a1f67a318d6ab22e67a, tested on Debian Sid with 5.15.2 and it works as described.
hebasto:
re-ACK 62b125fd197879f112322a1f67a318d6ab22e67a, only removed the unused `SubFeeFromAmountChanged` signal since my [previous](https://github.com/bitcoin-core/gui/pull/390#pullrequestreview-726531766) review.
meshcollider:
utACK 62b125fd197879f112322a1f67a318d6ab22e67a
Tree-SHA512: 932ca89ae578a1e1c426561400d87cf005c231944feaf0f662ff8d88f32bdd65a927a090ea41510a15f8ec0ebcd5529672e9917720eb5ea85f413f081e45d5bb
8169fc4e73a87331e02502fc24e293831765c8b1 qt, refactor: Fix code styling of moved InitExecutor class (Hennadii Stepanov)
c82165a55701fe4ff604d7f30163051cd47c2363 qt, refactor: Move InitExecutor class into its own module (Hennadii Stepanov)
dbcf56b6c6e939923673b3f07bed7bb3632dbeb1 scripted-diff: Rename BitcoinCore class to InitExecutor (Hennadii Stepanov)
19a1d008310f250b69b7aa764a9f26384d5a4a85 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 8169fc4e73a87331e02502fc24e293831765c8b1
ryanofsky:
Code review ACK 8169fc4e73a87331e02502fc24e293831765c8b1. Only change is switching from `m_executor` from pointer to optional type (thanks for update!)
Tree-SHA512: a0552c32d26d9acf42921eb12bcdf68f02d52f7183c688c43257b1a58679f64e45f193ee2d316850c7f0f516561e17abe989fe545bfa05e158ad3f4c66d19bca
4c43b7d41d11072f382f938379d21cd2e0bcbb47 contrib: use hkps://keys.openpgp.org to retrieve builder keys (fanquake)
Pull request description:
`hkps://hkps.pool.sks-keyservers.net` is essentially no-longer functional,
and a number of distributions and GPG tools have since switched to using
the `keys.openpgp.org` key server as their default.
See this Debian patch for additional context:
https://salsa.debian.org/debian/gnupg2/-/blob/debian/main/debian/patches/Use-hkps-keys.openpgp.org-as-the-default-keyserver.patch
Switch to using keys.openpgp.org in the CI as well.
ACKs for top commit:
MarcoFalke:
cr ACK 4c43b7d41d11072f382f938379d21cd2e0bcbb47
Zero-1729:
ACK 4c43b7d41d11072f382f938379d21cd2e0bcbb47
Tree-SHA512: e6c72b67778b76f81c659eee0e4195fea9e579587c64921affd35b9d46a077d4e8754b7fb85ca90a9a4bbc5cd5a47b0c6e4c9dbf9a335418a12f774d665e5a19
fabb72b contrib: Remove xpired 522739F6 key (MarcoFalke)
faeab66 contrib: Replace developer keys with list of pgp fingerprints (MarcoFalke)
Pull request description:
Having to host a copy of the keys in this repo was a common source of discussion and distraction, caused by problems such as:
* Outdated keys. Unclear whether and when to replace by fresh copies.
* Unclear when to add a key of a new developer or Gitian builder.
The problems are solved by
* Having no keys but only the fingerprints
* Adding a rule of thumb, when to add a new key
<strike>Moving the keys to a different repo solves none of these issues, but since the keys are not bound to releases or git branches of Bitcoin Core, they should live somewhere else.
Obviously, all keys are hosted and distributed on key servers, but were added to the repo solely for convenience and redundancy.
Moving the mirror of those keys to a different repo makes it less distracting to update them -- let's say -- prior to every major release.
I updated our `doc/release-process.md` to reflect the new location.
DEPENDS_ON https://github.com/bitcoin-core/gitian.sigs/pull/621
</strike>
Tree-SHA512: c00795a07603190e26dc4526f6ce11e492fb048dc7ef54b38f859b77dcde25f58ec4449f5cf3f85a5e9c2dd2743bde53f7ff03c8eccf0d75d51784a6b164e47d
bb822a7af86897a9b6a5d616f193c258e8e76729 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 bb822a7af86897a9b6a5d616f193c258e8e76729
Rspigler:
tACK bb822a7af86897a9b6a5d616f193c258e8e76729
jonatack:
ACK bb822a7af86897a9b6a5d616f193c258e8e76729 per `git diff 2854ddc bb822a7`
prayank23:
tACK bb822a7af8
meshcollider:
Code review ACK bb822a7af86897a9b6a5d616f193c258e8e76729
Tree-SHA512: f6dddc72a74e5667071ccd77f8dce578382e8e29e7ed6a0834ac2e114a6d3918b59c2f194f4079b3259e13d9ba3b4f405619940c3ecb7a1a0344615aed47c43d
607076d01bf23c69ac21950c17b01fb4e1130774 test: remove confusing `MAX_BLOCK_BASE_SIZE` (Sebastian Falbesoner)
4af97c74edcda56cd15523bf3a335adea2bad14a test: introduce `get_weight()` helper for CBlock (Sebastian Falbesoner)
a084ebe1330bcec15715e08b0f65319142927ad1 test: introduce `get_weight()` helper for CTransaction (Sebastian Falbesoner)
Pull request description:
This is a very late follow-up PR to #10618, which removed the constant `MAX_BLOCK_BASE_SIZE` from the core implementation about four years ago (see also #10608 in why it was considered confusing and superfluous).
Since there is also no point in still keeping it in the functional test framework, the PR switches to weight-based accounting on the relevant test code parts and use `MAX_BLOCK_WEIGHT` instead for the block limit checks. To prepare that, the first two commits introduce `get_weight()` helpers for the classes CTransaction and CBlock, respectively.
ACKs for top commit:
MarcoFalke:
review ACK 607076d01bf23c69ac21950c17b01fb4e1130774 🚴
Tree-SHA512: d59aa0b6b3dfd0a849b8063e66de275d252f705f99e25cd3bf6daec028b47d946777ee5b42a060f5283cb18e917ac073119c2c0e11bbc21211f69ef0a6ed335a
c75a0d4c57 Merge bitcoin/bitcoin#29177: build: Fix check whether `-latomic` needed (fanquake)
f670118cce Merge bitcoin/bitcoin#28851: build: Patch Qt to handle minimum macOS version properly (fanquake)
685ee8a46f Merge bitcoin/bitcoin#28884: doc: remove x86_64 build assumption from depends doc (fanquake)
47f6126504 Merge bitcoin/bitcoin#28881: doc: remove mention of missing bdb being a configure error (fanquake)
a9021db4ec Merge bitcoin/bitcoin#28777: doc: update docs for `CHECK_ATOMIC` macro (fanquake)
d5e15dfc5a Merge bitcoin/bitcoin#26839: Add support for RNDR/RNDRRS for AArch64 on Linux (Andrew Chow)
5aedcbfb43 Merge bitcoin/bitcoin#28778: depends: drop -O1 workaround from arm64 apple Qt build (fanquake)
95a8d8cfdc Merge bitcoin/bitcoin#21161: Fee estimation: extend bucket ranges consistently (glozow)
f4ea48e623 Merge bitcoin/bitcoin#28693: build: Include `config/bitcoin-config.h` explicitly in `util/trace.h` (fanquake)
f160e0dbb2 Merge bitcoin/bitcoin#28691: refactor: Remove CBlockFileInfo::SetNull (fanquake)
0278163aa3 Merge bitcoin/bitcoin#28697: fuzz: Increase merge -rss_limit_mb (fanquake)
90a1fb0e8d Merge bitcoin/bitcoin#28650: fuzz: Merge with -set_cover_merge=1 (fanquake)
f007abd19d Merge bitcoin/bitcoin#28459: build: add `-mbranch-protection=bti` (aarch64) to hardening flags (fanquake)
af8d12445a Merge bitcoin/bitcoin#28624: docs: fix typo (fanquake)
c740264da8 Merge bitcoin/bitcoin#28532: qt: enable` -ltcg` for windows under LTO (fanquake)
ccd3920d40 Merge bitcoin/bitcoin#28556: doc: fix link to developer-notes.md file in multiprocess.md (fanquake)
Pull request description:
## Issue being fixed or feature implemented
Batch of trivial backports
## What was done?
See commits
## How Has This Been Tested?
built locally; large combined merge passed tests locally
## Breaking Changes
Should be none
## Checklist:
_Go over all the following points, and put an `x` in all the boxes that apply._
- [ ] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_
ACKs for top commit:
UdjinM6:
utACK c75a0d4c57
Tree-SHA512: 035dc3fa9812c7f381946ae4798b8e729a58b38a090d94502a8d992e9cfaab3307173c602d7b782c637a79c5c41b62570dc73bb4bb367e4505a039964926181b
8bf1d06599 Merge bitcoin/bitcoin#29308: doc: update `BroadcastTransaction` comment (glozow)
2a77808596 Merge bitcoin-core/gui#789: Avoid non-self-contained Windows header (Hennadii Stepanov)
da371b830d Merge bitcoin/bitcoin#28870: depends: Include `config.guess` and `config.sub` into `meta_depends` (fanquake)
2e41562d81 Merge bitcoin/bitcoin#29219: fuzz: Improve fuzzing stability for ellswift_roundtrip harness (fanquake)
b091329599 Merge bitcoin/bitcoin#29211: fuzz: fix `connman` initialization (Ava Chow)
df42d41060 Merge bitcoin/bitcoin#29200: net: create I2P sessions using both ECIES-X25519 and ElGamal encryption (fanquake)
4cdd1a8a5d Merge bitcoin/bitcoin#29172: fuzz: set `nMaxOutboundLimit` in connman target (fanquake)
97012ea522 Merge bitcoin/bitcoin#28962: doc: Rework guix docs after 1.4 release (fanquake)
c70ff5d702 Merge bitcoin/bitcoin#28844: contrib: drop GCC MAX_VERSION to 4.3.0 in symbol-check (fanquake)
e6f19e7760 Merge bitcoin/bitcoin#29068: test: Actually fail when a python unit test fails (fanquake)
75e0334866 Merge bitcoin/bitcoin#28989: test: Fix test by checking the actual exception instance (Andrew Chow)
8cd85d311f Merge bitcoin/bitcoin#28852: script, assumeutxo: Enhance validations in utxo_snapshot.sh (Ryan Ofsky)
fd2e88d6f3 Merge bitcoin/bitcoin#26077: guix: switch from `guix environment` to `guix shell` (fanquake)
02741a7706 Merge bitcoin/bitcoin#28913: coins: make sure PoolAllocator uses the correct alignment (fanquake)
dfd53dabed Merge bitcoin/bitcoin#28902: doc: Simplify guix install doc, after 1.4 release (fanquake)
Pull request description:
## Issue being fixed or feature implemented
Batch of trivial backports
## What was done?
See commits
## How Has This Been Tested?
built locally; large combined merge passed tests locally
## Breaking Changes
Should be none
## Checklist:
_Go over all the following points, and put an `x` in all the boxes that apply._
- [ ] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_
ACKs for top commit:
UdjinM6:
utACK 8bf1d06599
knst:
utACK 8bf1d06599
Tree-SHA512: 506273e5a188f9ca74edf656e3cd338992192e6e97f68c89fc43e34be20fb7f211b48e4dfa8693727839a7920da8284509413c722f55774a428939c296dad517
fa67b8181c3ecf94395ecc50fd8acd436f1f8c3a Refactor: Remove unused FlatFilePos::SetNull (MarcoFalke)
Pull request description:
This is unused outside of tests and the default constructor. With C++11, it can be replaced by C++11 member initializers in the default constructor.
Beside removing unused code, this also makes it less fragile in light of uninitialized memory. (See also https://github.com/bitcoin/bitcoin/pull/26296#issuecomment-1477801767)
If new code needs to set this to null, it can use `std::optional`, or in the worst case re-introduce this method.
ACKs for top commit:
dergoegge:
Code review ACK fa67b8181c3ecf94395ecc50fd8acd436f1f8c3a
TheCharlatan:
ACK fa67b8181c3ecf94395ecc50fd8acd436f1f8c3a
john-moffett:
ACK fa67b8181c3ecf94395ecc50fd8acd436f1f8c3a
Tree-SHA512: 465c5e3eb4625405c445695d33e09a1fc5185c7dd1e766ba06034fb093880bfc65441d5334f7d9b20e2e417c2075557d86059f59d9648ca0e62a54c699c029b9
e646fa89f0 fix(ci): yet another 6356 follow-up (UdjinM6)
Pull request description:
## Issue being fixed or feature implemented
```
From https://github.com/dashpay/dash
* branch refs/pull/6348/head -> FETCH_HEAD
hint: You have divergent branches and need to specify how to reconcile them.
hint: You can do so by running one of the following commands sometime before
hint: your next pull:
hint:
hint: git config pull.rebase false # merge
hint: git config pull.rebase true # rebase
hint: git config pull.ff only # fast-forward only
hint:
hint: You can replace "git config" with "git config --global" to set a default
hint: preference for all repositories. You can also pass --rebase, --no-rebase,
hint: or --ff-only on the command line to override the configured default per
hint: invocation.
fatal: Need to specify how to reconcile divergent branches.
```
https://github.com/dashpay/dash/actions/runs/11506422850/job/32030325600
## What was done?
## How Has This Been Tested?
## Breaking Changes
## Checklist:
- [ ] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e tests
- [ ] I have made corresponding changes to the documentation
- [ ] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_
ACKs for top commit:
PastaPastaPasta:
Gosh, are you done yet? :D utACK e646fa89f0
Tree-SHA512: 533a2aaf73400fcb12b848d1496496667a1e7a89c17563b14f1c45c1624cff7d320563d6531d2b6b0e4b3cd638e2ff070bbd810bd88e75ce628de940b025fc12
9a79217756 Merge bitcoin/bitcoin#28227: test: check for specific bip157 disconnect reasons, add test coverage (fanquake)
a7a4603b8e Merge bitcoin-core/gui#755: Silence `-Wcast-function-type` warning (Hennadii Stepanov)
e216d0851d Merge bitcoin/bitcoin#27934: test: added coverage to estimatefee (fanquake)
34f0f56582 Merge bitcoin/bitcoin#28506: fuzz: Add missing PROVIDE_FUZZ_MAIN_FUNCTION guard to __AFL_FUZZ_INIT (fanquake)
ca0225c0fd Merge bitcoin/bitcoin#28480: fuzz: Don't use afl++ deferred forkserver mode (fanquake)
2b236ad07b Merge bitcoin/bitcoin#28460: fuzz: Use afl++ shared-memory fuzzing (fanquake)
52f036b316 Merge bitcoin/bitcoin#28427: index: coinstats reorg, fail when block cannot be reversed (fanquake)
6ad6f2f28d Merge bitcoin/bitcoin#28412: test: remove unused variables in `p2p_invalid_block` (fanquake)
43b88315e1 Merge bitcoin/bitcoin#28426: doc: s/--no-substitute/--no-substitutes in guix/INSTALL (fanquake)
1730a267ba Merge bitcoin/bitcoin#28386: test: remove fixed timeouts from feature_config_args (fanquake)
7f83db0d0c Merge bitcoin/bitcoin#28332: test: previous releases: speed up fetching sources with shallow clone (fanquake)
8490bf4b03 Merge bitcoin/bitcoin#28288: test: fix 'unknown named parameter' test in `wallet_basic` (fanquake)
8b8ff1c7d5 Merge bitcoin/bitcoin#28215: fuzz: fix a couple incorrect assertions in the `coins_view` target (fanquake)
c36f7d93fa Merge bitcoin/bitcoin#27401: tracepoints: Disables `-Wgnu-zero-variadic-macro-arguments` to compile without warnings (fanquake)
163020ef92 Merge bitcoin/bitcoin#28203: refactor: serialization simplifications (fanquake)
24e57da770 Merge bitcoin/bitcoin#28181: qa, doc: Fix comment (fanquake)
933a63e8fc Merge bitcoin/bitcoin#28145: valgrind: add suppression for bug 472219 (fanquake)
33766805eb Merge bitcoin/bitcoin#28124: fuzz: Re-enable symbolize=1 in ASAN_OPTIONS (fanquake)
621061459a Merge bitcoin/bitcoin#28099: contrib: move user32.dll from bitcoind.exe libs (fanquake)
Pull request description:
## Issue being fixed or feature implemented
Batch of trivial backports
## What was done?
See commits
## How Has This Been Tested?
built locally; large combined merge passed tests locally
## Breaking Changes
Should be none
## Checklist:
_Go over all the following points, and put an `x` in all the boxes that apply._
- [ ] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_
ACKs for top commit:
knst:
utACK 9a79217756
UdjinM6:
utACK 9a79217756
Tree-SHA512: 12d18abf28e3140bbb488fe912cf8e5c18a38aebaf09caad3120dc556116f348c4bbb40131f72c89edfbf48eb980c6947dde155e58e47c783f1d5b125aa6524b
7064eb9104 fix: 6356 follow-up (UdjinM6)
Pull request description:
## Issue being fixed or feature implemented
🙈
## What was done?
## How Has This Been Tested?
## Breaking Changes
## Checklist:
- [ ] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e tests
- [ ] I have made corresponding changes to the documentation
- [ ] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_
ACKs for top commit:
PastaPastaPasta:
Happens :) utACK 7064eb9104
Tree-SHA512: 5c1a5f154ba5698eb89d57bffa369f106ed9d6248c02ff975bdde995dfbe4276140360ec7d2b5d45c32e82e9451aa8db99d80d21f680298311772e27caa410b3
BACKPORT NOTE:
Prior work is done here: c338cd69d4 (diff-060e8fd790fc1c3e18c64327a7395bb5b2d6d57db9792cc666bd8d7354a40c0b)
Missing changes in tx_verify.h included in this PR, but changes in
src/test/sigopcount_tests.cpp and src/test/transaction_tests.cpp are
irrelevant because they are segwit-related
fa621ededdfe31a200b77a8787de7e3d2e667aec 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 fa621ededdfe31a200b77a8787de7e3d2e667aec
kristapsk:
ACK fa621ededdfe31a200b77a8787de7e3d2e667aec
jonatack:
ACK fa621ededdfe31a200b77a8787de7e3d2e667aec
Tree-SHA512: ea0720f32f823fa7f075309978672aa39773c6019d12b6c1c9d611fc1983a76115b7fe2a28d50814673bb6415c311ccc05b99d6e871575fb6900faf75ed17769
c76e398caa ci: check ff after merge into base branch for PRs (UdjinM6)
Pull request description:
## Issue being fixed or feature implemented
Check ff against the current state of PR's base branch instead of the one it had on PR creation.
## What was done?
## How Has This Been Tested?
## Breaking Changes
## Checklist:
- [ ] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e tests
- [ ] I have made corresponding changes to the documentation
- [ ] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_
ACKs for top commit:
PastaPastaPasta:
utACK; this makes sense to me c76e398caa
Tree-SHA512: ba68bc6454db81851e9356f27194e9f67a00935b51a0dba028902c8bed736e23021771220dcf45d2e1c350fa0705ecf3f0a4a78404dcab1152db837b611ac4e1
f8ca1357c8205ceff732dcfb0d2bad79b40b611b build: Fix check whether `-latomic` needed (Hennadii Stepanov)
Pull request description:
Clang >=15 still might need linking against `libatomic`.
We use `std::atomic<std::chrono::seconds>::compare_exchange_strong` in `net_processing.cpp`.
Addresses the https://github.com/bitcoin/bitcoin/pull/29165#discussion_r1440293694.
ACKs for top commit:
maflcko:
lgtm ACK f8ca1357c8205ceff732dcfb0d2bad79b40b611b
fanquake:
ACK f8ca1357c8205ceff732dcfb0d2bad79b40b611b
Tree-SHA512: ba8b6a88fd3471a206d068e8a000a053c99cb46d26bd04624418ddb066b3b9664a569ec8a1569af67c96b3e27f13dccbd5e24f985290ac072b6d74c92524e35d
05aca093819be276ac7d648472c6ed5c7d235cc5 build: Patch Qt to handle minimum macOS version properly (Hennadii Stepanov)
Pull request description:
This PR is:
- required to [switch](https://github.com/bitcoin/bitcoin/pull/28622) to macOS 14 SDK (Xcode 15).
- an alternative to https://github.com/bitcoin/bitcoin/pull/28732 and https://github.com/bitcoin/bitcoin/pull/28775.
Qt relies on the `__MAC_OS_X_VERSION_MIN_REQUIRED` macro, which is set in the `AvailabilityInternal.h` SDK header to
the value provided by the Clang driver from the `-mmacos-version-min` / `-mmacosx-version-min` option.
Xcode 12 SDK expects the OS-specific `__ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__` macro:
```c++
#ifndef __MAC_OS_X_VERSION_MIN_REQUIRED
#ifdef __ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__
/* compiler for Mac OS X sets __ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__ */
#define __MAC_OS_X_VERSION_MIN_REQUIRED __ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__
#endif
#endif /* __MAC_OS_X_VERSION_MIN_REQUIRED*/
```
In the other hand, Xcode 15 SDK expects a general `__ENVIRONMENT_OS_VERSION_MIN_REQUIRED__` macro:
```c++
#ifndef __MAC_OS_X_VERSION_MIN_REQUIRED
#if defined(__has_builtin) && __has_builtin(__is_target_os)
#if __is_target_os(macos)
#define __MAC_OS_X_VERSION_MIN_REQUIRED __ENVIRONMENT_OS_VERSION_MIN_REQUIRED__
#define __MAC_OS_X_VERSION_MAX_ALLOWED __MAC_14_0
#endif
#elif __ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__
#define __MAC_OS_X_VERSION_MIN_REQUIRED __ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__
#define __MAC_OS_X_VERSION_MAX_ALLOWED __MAC_14_0
#endif /* __has_builtin(__is_target_os) && __is_target_os(macos) */
#endif /* __MAC_OS_X_VERSION_MIN_REQUIRED */
```
The latter macro is not provided by LLVM Clang until c8e2dd8c6f, which is available in Clang 17.
The suggested patch makes Qt "borrow" the `__MAC_OS_X_VERSION_MIN_REQUIRED` value from `MAC_OS_X_VERSION_MIN_REQUIRED`, which is set in the `AvailabilityMacros.h` SDK header.
ACKs for top commit:
maflcko:
lgtm ACK 05aca093819be276ac7d648472c6ed5c7d235cc5
Tree-SHA512: 8891aefde4b8a48885abf0648f4ec71a22f7fcfca1e17ebb8c70ce1ef44751ea5db6b8b652de6ee8a716ca5f96f720fef01600bc23986162d0146c946e2e8743
821a8a11256ccf26fe8b0255cea4ec04dddd8f18 doc: remove x86_64 build assumption from depends doc (fanquake)
Pull request description:
This dates from the introduction of depends, and has not been the case for some time now.
ACKs for top commit:
maflcko:
lgtm ACK 821a8a11256ccf26fe8b0255cea4ec04dddd8f18
hebasto:
ACK 821a8a11256ccf26fe8b0255cea4ec04dddd8f18.
theuni:
ACK 821a8a11256ccf26fe8b0255cea4ec04dddd8f18
Tree-SHA512: 640967a3e6dfab495fd733d3379aa916ac7f67e89a92ef6a94c3bea0494dc7921a9d7485e1b90a1beab00548b575cdab8fb08eb9267dcc5e890cc796ae1b6875
30bd4b1e4aee00edbe77da7c20bf80e28f0561cc doc: remove mention of missing bdb being a configure error (fanquake)
Pull request description:
This is no-longer the case, unless you're passing additional flags, which is not the case in this example.
ACKs for top commit:
maflcko:
lgtm ACK 30bd4b1e4aee00edbe77da7c20bf80e28f0561cc
TheCharlatan:
ACK 30bd4b1e4aee00edbe77da7c20bf80e28f0561cc
hebasto:
ACK 30bd4b1e4aee00edbe77da7c20bf80e28f0561cc.
Tree-SHA512: b3730546d7ff1f49854b88e710c72c4f6e4b6d238147599d4c4e4adeeb256424c2096635f6c51dcfe2e5a9c1155c1c9915fe03a09c5c38605bee2722756c8f6e
ebc7063c80135dd6f3e7b9418e8f4bf217bd8db7 doc: update docs for CHECK_ATOMIC macro (fanquake)
Pull request description:
Clarify that supported versions of GCC are not affected, and that Clang
prior to version 15 still requires the explicit `-latomic` linking, when
compiling for 32-bit.
ACKs for top commit:
hebasto:
ACK ebc7063c80135dd6f3e7b9418e8f4bf217bd8db7.
Tree-SHA512: 6044dc28547431cfde7e89b663b5f9a86a4cb801212a21c3dbb18a1c41a53640480c3e4e944050dc3ec4cded9bc4c1f8eae8dbb60596289fef49bb13a8b53b76
aee5404e02e203a256c1a97b629b9b107cc8bb07 Add support for RNDR/RNDRRS for aarch64 on Linux (John Moffett)
Pull request description:
This checks whether the ARMv8.5-A optional TRNG extensions [RNDR](https://developer.arm.com/documentation/ddi0601/2022-12/AArch64-Registers/RNDR--Random-Number) and [RNDRRS](https://developer.arm.com/documentation/ddi0601/2022-12/AArch64-Registers/RNDRRS--Reseeded-Random-Number) are available and, if they are, uses them for random entropy purposes.
They are nearly functionally identical to the x86 RDRAND/RDSEED extensions and are used in a similar manner.
Currently, there [appears to be](https://marcin.juszkiewicz.com.pl/download/tables/arm-socs.html) only one actual hardware implementation -- the Amazon Graviton 3. (See the `rnd` column in the link.) However, future hardware implementations may become available.
It's not possible to directly query for the capability in userspace, but the Linux kernel [added support](1a50ec0b3b) for querying the extension via `getauxval` in version 5.6 (in 2020), so this is limited to Linux-only for now.
Reviewers may want to launch any of the `c7g` instances from AWS to test the Graviton 3 hardware. Alternatively, QEMU emulates these opcodes for `aarch64` with CPU setting `max`.
Output from Graviton 3 hardware:
```
ubuntu@ip:~/bitcoin$ src/bitcoind -regtest
2023-01-06T20:01:48Z Bitcoin Core version v24.99.0-3670266ce89a (release build)
2023-01-06T20:01:48Z Using the 'arm_shani(1way,2way)' SHA256 implementation
2023-01-06T20:01:48Z Using RNDR and RNDRRS as additional entropy sources
2023-01-06T20:01:48Z Default data directory /home/ubuntu/.bitcoin
```
Graviton 2 (doesn't support extensions):
```
ubuntu@ip:~/bitcoin$ src/bitcoind -regtest
2023-01-06T20:05:04Z Bitcoin Core version v24.99.0-3670266ce89a (release build)
2023-01-06T20:05:04Z Using the 'arm_shani(1way,2way)' SHA256 implementation
2023-01-06T20:05:04Z Default data directory /home/ubuntu/.bitcoin
```
This partially closes#26796. As noted in that issue, OpenSSL [added support](https://github.com/openssl/openssl/pull/15361) for these extensions a little over a year ago.
ACKs for top commit:
achow101:
ACK aee5404e02e203a256c1a97b629b9b107cc8bb07
laanwj:
Tested ACK aee5404e02e203a256c1a97b629b9b107cc8bb07
Tree-SHA512: 1c1eb345d6690f5307a87e9bac8f06a0d1fdc7ca35db38fa22192510a44289a03252e4677dc7cbf731a27e6e3a9a4e42b6eb4149fe063bc1c905eb2536cdb1d3
a5e39d325da4eeb9273fb7c919fcbfbc721ed75d Fee estimation: extend bucket ranges consistently (Anthony Towns)
Pull request description:
When calculating a median fee for a confirmation target at a particular threshold, we analyse buckets in ranges rather than individually in case some buckets have very little data. This patch ensures the breaks between ranges are independent of the the confirmation target.
Fixes#20725
ACKs for top commit:
ismaelsadeeq:
Code review ACK a5e39d325da4eeb9273fb7c919fcbfbc721ed75d
glozow:
btw what I meant by [this](https://github.com/bitcoin/bitcoin/pull/21161#pullrequestreview-1350258467) was ACK a5e39d325da4eeb9273fb7c919fcbfbc721ed75d
jonatack:
Initial ACK a5e39d325da4eeb9273fb7c919fcbfbc721ed75d
Tree-SHA512: 0edf4e56717c4ab8d4ab0bc0f1d7ab36a13b99de12f689e55c9142c6b81691367ffd8df2e8260c5e14335310b1a51770c6c22995db31109976239befcb558ef8
6bdff429ec17eae4138c3af1e21de3ec46f4ab13 build: Include `config/bitcoin-config.h` explicitly in `util/trace.h` (Hennadii Stepanov)
Pull request description:
The `ENABLE_TRACING` macro is expected to be defined in the `config/bitcoin-config.h` header.
Therefore, the current code is error-prone as it depends on whether the `config/bitcoin-config.h` header was included before or not.
This bug was noticed while working on CMake [stuff](https://github.com/hebasto/bitcoin/pull/37).
ACKs for top commit:
fanquake:
ACK 6bdff429ec17eae4138c3af1e21de3ec46f4ab13
Tree-SHA512: 22c4fdeb51628814050eb99a83db4268a4f3106207eeef918a07214bbc52f2b22490f6b05fcb96216f147afa4197c51102503738131e2583e750b6d195747a49
fac36b94ef32567c0f10b605a3a441d11559e56e refactor: Remove CBlockFileInfo::SetNull (MarcoFalke)
Pull request description:
Seems better to use C++11 member initializers and then let the compiler figure out how to construct objects of this class.
ACKs for top commit:
stickies-v:
ACK fac36b94ef32567c0f10b605a3a441d11559e56e
pablomartin4btc:
ACK fac36b94ef32567c0f10b605a3a441d11559e56e
theStack:
LGTM ACK fac36b94ef32567c0f10b605a3a441d11559e56e
Tree-SHA512: aee741c8f668f0e5b658fc83f4ebd196b43fead3dd437afdb0a2dafe092ae3d559332b3d9d61985c92e1a59982d8f24942606e6a98598c6ef7ff43697e858725
fa21535551e300eaa988d209ad64cdc17fd7f66b fuzz: Increase merge -rss_limit_mb (MarcoFalke)
Pull request description:
For some reason, the limit is hit. (Presumably due to `-set_cover_merge=1` eating more memory, or by simply having more fuzz inputs).
Fix it by increasing it for the merge operation.
ACKs for top commit:
dergoegge:
ACK fa21535551e300eaa988d209ad64cdc17fd7f66b
hebasto:
ACK fa21535551e300eaa988d209ad64cdc17fd7f66b, considering the discussion in https://github.com/bitcoin-core/qa-assets/pull/155.
Tree-SHA512: 4fed0f254eccc6fe0b53656bc345ff898b13811dc39387387317d34b521ab77cee03d82b0896dd92d253b7546b6a7e4bdcd478749f47064374ab44ad759ab9ff
c96f9e0285 feat: only require reindexing when the index was off going to off (pasta)
Pull request description:
## What was done?
It does not seem reasonable that we need to reindex when turning indexes off. this logic was introduced in e0781095f0
## How Has This Been Tested?
Hasn't
## Breaking Changes
None, basically
## Checklist:
_Go over all the following points, and put an `x` in all the boxes that apply._
- [ ] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_
ACKs for top commit:
knst:
utACK c96f9e0285
Tree-SHA512: a5b634b9fbb6632b35286055a8e5b8db340738b7327a7d860e645ec1f6ef5b5cddc140a3e1c51b09c2f47db375c83e7c53fe5eaf221ab8df1d173caf50fa862d
fa1eb0ecaef14d428812f956082d29ab134fc728 test: Make the unlikely race in p2p_invalid_messages impossible (MarcoFalke)
Pull request description:
After `add_p2p_connection` both sides have the verack processed.
However the pong from conn in reply to the ping from the node has not
been processed and recorded in totalbytesrecv.
Flush the pong from conn by sending a ping from conn.
This should make the unlikely race impossible.
ACKs for top commit:
mzumsande:
ACK fa1eb0ecaef14d428812f956082d29ab134fc728
pinheadmz:
ACK fa1eb0ecaef14d428812f956082d29ab134fc728
Tree-SHA512: 44166587572e8c0c758cac460fcfd5cf403b2883880128b13dc62e7f74ca5cb8f145bb68a903df177ff0e62faa360f913fd409b009d4cd1360f1f4403ade39ae
8fcbdadfad8a9b3143527141ff37e5fe1e87f3b3 util: fix argsman dupe key error (willcl-ark)
Pull request description:
fixes#22638
Make GUI "Settings file could not be read. Do you want to reset settings to default values?" dialog actually clear all settings instead of partially keeping them when `settings.json` contains duplicate keys. This change has no effect on `bitcoind` because it treats a corrupt `settings.json` file as a hard error and doesn't attempt to modify it.
If we find a duplicate key and error, clear `values` before returning so that `WriteSettings()` will write an empty file, therefore clearing it.
This aligns with GUI behaviour added in 1ee6d0b.
The test added only checks that `values` is empty after a duplicate key is detected. This paves the way for the `abort` option in the GUI to properly clear `settings.json`, if the user selects the option, but the test does not currently check this entire mechanism (e.g. the file contents).
ACKs for top commit:
TheCharlatan:
Code review ACK 8fcbdadfad8a9b3143527141ff37e5fe1e87f3b3
ryanofsky:
Code review ACK 8fcbdadfad8a9b3143527141ff37e5fe1e87f3b3. Thanks for the fix! I would maybe update the PR description or add to it to describe behavior change of this PR:
Tree-SHA512: a5fd49b30ede0a24188623192825bccb952e427cc35f96ff9bfdc737361dcc35ac6480589ddf7f0ddeaebd34361bdaee31e7a91f2c0d857e4ff682614bb6bc04
54c4d03578c5842f19bf8bc68aca5faf8beed5c3 doc: Show how less noisy clang-tidy output can be achieved (TheCharlatan)
Pull request description:
Adds a paragraph to the clang-tidy section explaining how to de-noise its output. By default clang-tidy will print errors arrising from included headers in leveldb and other dependencies. By passing `--enable-suppress-external-warnings` flag to configure, errors arising from external dependencies are suppressed. Additional errors arrising from internal dependencies such as leveldb are suppressed by passing the `src/.bear-tidy-config` configuration file to bear. This file includes exclusionary rules for leveldb.
ACKs for top commit:
MarcoFalke:
utACK 54c4d03578c5842f19bf8bc68aca5faf8beed5c3
Tree-SHA512: c3dd8fb0600157582a38365a587e02e1d249fb246d6b8b4949a800fd05d3473dee49e2a4a556c60e51d6508feff810024e55fe09f5a0875f560fde30f3b6817c
5c938e74cfdf6b89c6c4d5cce2fd07cbfc8b29c2 Use string interpolation for default value of -listen (ekzyis)
Pull request description:
This is a refactoring change. So I have read the following and will try to answer why this change should be accepted
> * Refactoring changes are only accepted if they are required for a feature or
bug fix or **_otherwise improve developer experience significantly_**. For example,
most "code style" refactoring changes require a thorough explanation why they
are useful, what downsides they have and why they *significantly* improve
developer experience or avoid serious programming bugs. Note that code style
is often a subjective matter. Unless they are explicitly mentioned to be
preferred in the [developer notes](/doc/developer-notes.md), stylistic code
changes are usually rejected.
I have noticed in https://github.com/bitcoin/bitcoin/pull/26899#discussion_r1086731856 that the helper message for `-listen` does not use string interpolation.
That confused me and I wasn't sure what the reasons for that are. So it could be argued this confusion (by possibly many people in the past and in the future) may already be enough to accept this change.
However, not accepting this means that if `DEFAULT_LISTEN` is ever changed, this helper message will still use the old value (however unlikely that may be).
Therefore, this PR makes the helper message consistent with how other helper messages are implemented (using string interpolation) which leads to less confusion and prevents possibly wrong documentation in the future.
ACKs for top commit:
stratospher:
ACK 5c938e7.
vasild:
ACK 5c938e74cfdf6b89c6c4d5cce2fd07cbfc8b29c2
kristapsk:
cr utACK 5c938e74cfdf6b89c6c4d5cce2fd07cbfc8b29c2
Tree-SHA512: 8905f548ff399967966e67b29962821c28fd2620ff788c2b2bdfd088bbca75457dc63e933ad1985f93580508a65b91930fe4b2d97a2e0a7d83a3748b9ea2c26f
faa671591f9c83ef0fb5afea151a1907c28f024b test: Use self.wait_until over wait_until_helper (MarcoFalke)
Pull request description:
`wait_until_helper` is a "private" helper, not intended to be used directly, because it doesn't scale the timeout with the timeout factor. Fix this by replacing it with a call to `self.wait_until`, which does the scaling.
ACKs for top commit:
theStack:
Code-review ACK faa671591f9c83ef0fb5afea151a1907c28f024b
Tree-SHA512: 70705f309f83ffd6ea5d090218195d05b868624d909106863372f861138b5a70887070b25beb25044ae1b44250345e45c9cc11191ae7aeca2ad37801a0f62f61
8847ce44e0713350a6d3524f62eaeb10ba548bae util: add missing include and fix function signature (Cory Fields)
Pull request description:
ping hebasto
Discovered while testing pre-compiled header support with CMake: https://github.com/theuni/bitcoin/commits/cmake-pch-poc. Compilation of that branch fails without this fix and succeeds with it.
Similar to the fix in #27144.
The problem of having a default argument in the definition was masked by the missing include. Using PCH forces that include, so we end up with the compiler error we should've been getting all along.
ACKs for top commit:
fanquake:
ACK 8847ce44e0713350a6d3524f62eaeb10ba548bae
Tree-SHA512: 5eb9a6691ee37cbc5033a48aedcbf5c93af3b234614ae14c3fcc858f1ee505f630ad68f8bbb69ffa280080c8d0f91451362cb3819290b741ce906b2b3224a622
29b62c01c8d211475ea9dd1a1093820f0a86c06d valgrind: remove libsecp256k1 suppression (fanquake)
Pull request description:
I am no-longer been able to recreate this issue, atleast after the most recent libsecp256k1 changes. Can someone else confirm?
ACKs for top commit:
MarcoFalke:
lgtm ACK 29b62c01c8d211475ea9dd1a1093820f0a86c06d
sipa:
utACK 29b62c01c8d211475ea9dd1a1093820f0a86c06d
Tree-SHA512: e61e5b88ac2af3c779f23d999938bec4497f6433a029c07dd57a9481fe9cc104d8d8f63586910b29f41e66bbf0ed094bc7539c0df84754a1783c4b1b15af072e
84ca5b349ecc2ad083bb39352e5d5ae731fb1622 doc: mention sanitizer suppressions in developer docs (fanquake)
Pull request description:
Should be enough to close#17834.
ACKs for top commit:
MarcoFalke:
lgtm ACK 84ca5b349ecc2ad083bb39352e5d5ae731fb1622
Tree-SHA512: 233c688a3cef1006c9a00f7b7a52fd6ee0ec150367e5e56904b6f1bbdca21b9217c69f8fcf653a4943613d12c3178a39f761b25eb24fc1954a563cfb1f832f5e
