fa275e1539 test: Fix intermittent feature_taproot issue (MarcoFalke)
Pull request description:
The nodes might disconnect (e.g. due to "Timeout downloading block" https://cirrus-ci.com/task/5313800947630080?command=ci#L1763) and the test fails to continue.
Fix that by reconnecting the nodes.
ACKs for top commit:
laanwj:
code review ACK fa275e1539
Tree-SHA512: 2871183c8058d8292c9c4ef56ea3d19d5616ca712ebdaabb6609f8c9cd2e16c9ac2ce26aa1e94b346872b7b6fec56b59af151af83de3a5aa08bed01bfcc7187a
fa8abdc995 rpc: Use FeeModes doc helper in estimatesmartfee (MarcoFalke)
Pull request description:
Not sure why this doesn't use the doc helper, probably an oversight?
ACKs for top commit:
laanwj:
Code review ACK fa8abdc995
Tree-SHA512: 1f2dc8356e3476ddcf9cafafa7f9865ad95bed1e3067c0edab8e3c483e374bdbdbecc066167554b4a1b479e28f6a52c4ae6a75a70c67ee4e1ff4f3ba36b04001
4e28753f60 feestimator: encapsulate estimation file logic (Antoine Poinsot)
e8ea6ad9c1 init: don't create a CBlockPolicyEstimator if we don't relay transactions (Antoine Poinsot)
86ff2cf202 Remove the remaining fee estimation globals (Antoine Poinsot)
03bfeee957 interface: remove unused estimateSmartFee method from node (Antoine Poinsot)
Pull request description:
If the `blocksonly` mode is turned on after running with transaction
relay enabled for a while, the fee estimation will serve outdated data
to both the internal wallet and to external applications that might be
feerate-sensitive and make use of `estimatesmartfee` (for example a
Lightning Network node).
This has already caused issues (for example https://github.com/bitcoin/bitcoin/issues/16840 (C-lightning), or https://github.com/lightningnetwork/lnd/issues/2562 (LND)) and it seems prudent to fail rather than to give inaccurate values.
This fixes#16840, and closes#16890 which tried to fix the symptoms (RPC) but not the cause as mentioned by sdaftuar :
> If this is a substantial problem, then I would think we should take action to protect our own wallet users as well (rather than hide the results of what our fee estimation would do!).
ACKs for top commit:
MarcoFalke:
re-ACK 4e28753f60👋
jnewbery:
utACK 4e28753f60
Tree-SHA512: c869cf03b86d8194002970bbc84662dae76874967949b9be0d9a4511a1eabcb1627c38aca3154da9dcece1a4c49ec02bd4f9fcca2ec310986e07904559e63ba8
fa0f415709 net: Assume that SetCommonVersion is called at most once per peer (MarcoFalke)
Pull request description:
This restores the check removed in https://github.com/bitcoin/bitcoin/pull/17785#discussion_r503224381
Instead of using `error`, which was used previously, it uses a newly introduced `Assume()`. `error` had several issues:
* It logs unconditionally to the debug log
* It doesn't abort the program when the error is hit in tests
ACKs for top commit:
practicalswift:
cr ACK fa0f415709: patch looks correct
jnewbery:
utACK fa0f415709
Tree-SHA512: cd7424a9485775e8c7093b725f8f52a90d47485185e79bac80f7810e450d0b3fda608d8805e9239094929f7bad2dca3fe772fb78ae606c2399d15405521e136b
6690adba08 Warn when binaries are built from a dirty branch. (Tyler Chambers)
Pull request description:
- Adjusted `--version` flag behavior in bitcoind and bitcoin-wallet to have the same behavior.
- Added `--version` flag to bitcoin-tx to match.
- Added functionality in gen-manpages.sh to error when attempting to generate man pages for binaries built from a dirty branch.
mitigates problem with issue #20412
ACKs for top commit:
laanwj:
Tested ACK 6690adba08
Tree-SHA512: b5ca509f1a57f66808c2bebc4b710ca00c6fec7b5ebd7eef58018e28e716f5f2358e36551b8a4df571bf3204baed565a297aeefb93990e7a99add502b97ee1b8
c23f6f84ef Add depends qt fix for ARM macs (Jonas Schnelli)
Pull request description:
With this, depends builds fine on macOS 11 on an Apple Silicon Mac (ARM64).
ACKs for top commit:
laanwj:
Code review ACK c23f6f84ef
Tree-SHA512: a8354cec99969cff9e7dab150c335050ddb4b3c93a9f12a4db5e8046f02b11ce692ac17c2b96cbbe7f380c1aa110b15b8d6d48d51bc9c560282c702e99fd8a8d
1816327e53 p2p: Put disconnecting logs into BCLog::NET category (Hennadii Stepanov)
Pull request description:
It's too noisy:
```
$ cat debug.log | wc -l
28529
$ cat debug.log | grep "Disconnecting and discouraging peer" | wc -l
10177
```
ACKs for top commit:
MarcoFalke:
noban, addnode and local peers are still unconditionally logged (as they should), but this one can go into a category, so cr-ACK 1816327e53
practicalswift:
ACK 1816327e53 for the reasons MarcoFalke gave above.
ajtowns:
ACK 1816327e53
Tree-SHA512: c312c1009090840659b2cb1364d8ad9b6ab8e742fc462aef169996d93c76c248507639a00257ed9d73a6916c01176b1793491b2305e92fdded5f9de0935b6ba6
ed1bbcefea contrib: add MACHO tests to symbol-check tests (fanquake)
5bab08df17 contrib: Add test for ELF symbol-check (Wladimir J. van der Laan)
Pull request description:
Check both failure cases:
- Use a glibc symbol from a version that is too new
- Use a symbol from a library that is not in the allowlist
And also check a conforming binary.
Adding a similar check for Windows PE can be done in a separate PR.
ACKs for top commit:
fanquake:
ACK ed1bbcefea
Tree-SHA512: fd437612e003922465fe1396efa1fa3a64bd1c7b0a514d2a0a7a0caaaa9fb5cb43e0ed7caec15eb0a3508692c9eb3212d7ba3c7e8180b942dd3e50616ad6e557
cb0b7125c1 doc: libbitcoinconsensus: add missing error code description, fix NBitcoin link (Sebastian Falbesoner)
Pull request description:
This PR improves the libbitcoinconsensus description in `shared-libraries.md` in two ways:
* adds the missing error code description for `bitcoinconsensus_ERR_INVALID_FLAGS` (introduced by commit 5ca8ef299a, PR #8976)
* updates and fixes the link to the NBitcoin implementation (introduced by commit 3361edd010, PR #6430)
* the owner of the `NBitcoin` github repository changed from `NicolasDorier` to `MetacoSA` (redirection still worked though)
* instead of dynamically referring to a file in master with a fixed line number (which is obviously always quickly outdated), use a permalink with a file numbers area
ACKs for top commit:
MarcoFalke:
cr ACK cb0b7125c1
harding:
Code (documentation) review ACK cb0b7125c1. Text is clear and seems accurate, and the link checks out.
Tree-SHA512: 9840458db6fb40e71c9852104aefcec5abbaf5054c6123701181dd477cea8c81d3647f376b67692159adf577c9b6305b05b784728bf9f14a753fab5898075a4e
fa6af31227 test: Document why syncwithvalidationinterfacequeue is needed in tests (MarcoFalke)
fa135a13b8 Revert "test: Add missing sync_all to wallet_balance test" (MarcoFalke)
Pull request description:
syncwithvalidationinterfacequeue is a hidden test-only RPC, so it should not be used when it is not needed. Thus, either remove it or explain why it is needed.
ACKs for top commit:
fjahr:
Code review ACK fa6af31227
Tree-SHA512: de30db4ab521184091ee5beeab02989138cf7cf05088f766a2fb106151b239310b63d5380cb79e2a072f72c5ae9513aecae8eb9c1c7be713771585c3cb04d63a
12dcdaaa54 Don't make "in" parameters look like "out"/"in-out" parameters: pass by ref to const instead of ref to non-const (practicalswift)
Pull request description:
Don't make "in" parameters look like "out"/"in-out" parameters: pass by ref to const instead of ref to non-const.
ACKs for top commit:
MarcoFalke:
cr ACK 12dcdaaa54
fjahr:
Code review ACK 12dcdaaa54
hebasto:
ACK 12dcdaaa54, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: bf9e476dc27bfbd856b42cc4720fb2d4b74e64039776ce067691aaf536360b36902ef72988c7914505df5bf0ed2f1557e0eba52b87935d3a17db865ea17f6093
773c42b265 tests: Test that a fully signed tx given to signrawtx is unchanged (Andrew Chow)
Pull request description:
Tests that a fully signed transaction given to `signrawtransactionwithwallet` is both unchanged and marked as complete. This tests for a regression in 0.20 where the transaction would not be marked as complete.
ACKs for top commit:
MarcoFalke:
ACK 773c42b265 🕶
Tree-SHA512: b32afd5f2667c817ee59c37af78cb37352e7c22d0271833d446fd61f11b354974694128a3b408f214db459caef3fd67ecdbf1664280ac2de602215f05aa4a6de
fac7ab1d5b refactor: Use C++17 std::array where possible (MarcoFalke)
Pull request description:
Using the C++11 std::array with explicit template parameters is problematic because overshooting the size will fill the memory with default constructed types.
For example,
```cpp
#include <array>
#include <iostream>
int main()
{
std::array<int, 3> a{1, 2};
for (const auto& i : a) {
std::cout << i << std::endl; // prints "1 2 0"
}
}
```
ACKs for top commit:
jonasschnelli:
Code Review ACK fac7ab1d5b
practicalswift:
cr ACK fac7ab1d5b
vasild:
ACK fac7ab1d
promag:
Code review ACK fac7ab1d5b.
Tree-SHA512: ef7e872340226e0d6160e6fd66c6ca78b2ef9c245fa0ab27fe4777aac9fba8d5aaa154da3d27b65dec39a6a63d07f1063c3a8ffb667a98ab137756a1a0af2656
e373959d6f Android : Ensure pic build for bdb (Block Mechanic)
Pull request description:
This pr ensures android builds for the BDB dependency have the pic flag enabled. Android builds were failing to link with reloc errors.
ACKs for top commit:
laanwj:
Code review ACK e373959d6f
jonasschnelli:
utACK e373959d6f
Tree-SHA512: 68319ed7cc0bd295eaa87dd53ba051daeb1456bc3ab9b48ca0c4b831a9c8da1073480478efde73689f0e403e37409a8459229264656f05ba5fef6c257a74f977
faa05854f8 util: Remove probably misleading TODO (MarcoFalke)
fac5efe730 util: Add Assume() identity function (MarcoFalke)
fa861569dc util: Allow Assert(...) to be used in all contexts (practicalswift)
Pull request description:
This is needed for #20138. Please refer to the added documentation for motivation.
ACKs for top commit:
practicalswift:
cr ACK faa05854f8
jnewbery:
utACK faa05854f8
hebasto:
ACK faa05854f8, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: 72165fbd898b92ab9a79b070993fa1faa86c2e3545b6645e72c652bda295d5107bc298d0482bf3aaf0926fc0c3e6418a445c0e073b08568c44231f547f76a688
86e6add5ca doc: rename CODEOWNERS to REVIEWERS (Adam Jonas)
Pull request description:
This PR renames the CODEOWNERS file to REVIEWERS and works with DrahtBot to leave a comment requesting a review.
Testing of the functionality was done on https://github.com/adamjonas/bitcoin-codeowners-sandbox/pulls.
~EDIT: [after further testing of a fake organization](https://github.com/jonasorg/bitcoin-codeowners-sandbox-org/pulls), it appears that in order to be automatically tagged, the reviewer requires write level permissions. This is undocumented and will obviously close the circle of who can be tagged and so this PR reverts the addition of the file in #18949.~
~This removes a line not being parsed in the CODEOWNERS file introduced in #18949. While the pattern [checked out](https://github.com/bitcoin/bitcoin/pull/18949#issuecomment-648859076) using `git ls-files`, it is preventing the CODEOWNERS file from automatically tagging reviewers. For future modifications to this file, note that [any line failing to parse causes the entire file to stop identifying codeowners](http://www.benjaminoakes.com/git/2018/08/10/Testing-changes-to-GitHub-CODEOWNERS/#:~:text=warning).~
~I experimented in a [sandbox repo](https://github.com/adamjonas/bitcoin-codeowners-sandbox/pulls) to discover the offending line and this change appears to fix the problem.~
ACKs for top commit:
laanwj:
Doesn't look like github is going to fix this any time soon and besides REVIEWERS is a better name so ACK 86e6add5ca
Tree-SHA512: b3425eca4be62f829f0edcfa08232df310680835c2f60e9fa36956a6b59a2b0cd0ab580c572c87affccd843fbd849f4f99e4b3d9d7b6070ab117953f04e17f5a
d3ef947524 build: Check that Homebrew's berkeley-db4 package is actually installed (Hennadii Stepanov)
Pull request description:
On master (a0489f3472) the `configure` script is not able to determine that Homebrew's `berkeley-db4` package is uninstalled. This causes a compile error on macOS.
With this PR, and with the [uninstalled](https://stackoverflow.com/questions/20802320/detect-if-homebrew-package-is-installed) `berkeley-db4` package:
```
% ./configure -q
configure: error: libdb_cxx headers missing, Bitcoin Core requires this library for BDB wallet support (--without-bdb to disable BDB wallet support)
```
Related #20478.
ACKs for top commit:
promag:
Tested ACK d3ef947524.
willcl-ark:
tACK d3ef947524
jonasschnelli:
utACK d3ef947524
Tree-SHA512: 8dc532e08249ec63bd357594aa458d314b6e8537fc63f5b1d509c84d0d71d5b1f70172caa1a7efe2fc8af31c829e7982a0695cf3fbe5cbc477019550269915e1
2f6fe4e4e9 ci: Build with --enable-werror by default, and document exceptions (Hennadii Stepanov)
Pull request description:
This PR prevents introducing of new compiler warnings in the master branch, e.g., #19986, #20162.
ACKs for top commit:
practicalswift:
cr ACK 2f6fe4e4e9: patch looks correct
MarcoFalke:
re-ACK 2f6fe4e4e9🏏
vasild:
ACK 2f6fe4e
Tree-SHA512: 23b5feb5bc472658c992d882ef61af23496f25adaa19f9c79bfaef5d2db273d44981aa93b1631a7d37cb58755283c1dacf3f2d68e501522d3fa8c965ab646d19
Tests that a fully signed transaction given to
signrawtransactionwithwallet is both unchanged and marked as complete.
This tests for a regression in 0.20 where the transaction would not be
marked as complete.
cadb77a6ab net: Add compat.h header for htonl function (Hennadii Stepanov)
f796f0057b net: Drop unneeded headers when compat.h included (Hennadii Stepanov)
467c346448 net: Drop unneeded Windows headers in compat.h (Hennadii Stepanov)
Pull request description:
It is the `compat.h` header's job to provide platform-agnostic interfaces for internet operations.
No need in `#include <arpa/inet.h>` scattered around.
ACKs for top commit:
practicalswift:
re-ACK cadb77a6ab: patch looks even better
laanwj:
Code review ACK cadb77a6ab
Tree-SHA512: 625ff90b2806310ab856a6ca1ddb6d9a85aa70f342b323e8525a711dd12219a1ecec8373ec1dca5a0653ffb11f9b421753887b25615d991ba3132c1cca6a3c6e
2c69381f3d Removed redundant git pull from appveyor config. (Aaron Clauson)
8b99e609e7 Adjusted msvc compiler and linker settings to remove optimisations that are causing sporadic ABI issues on Visual Studio updates. (Aaron Clauson)
Pull request description:
The motivation for this PR is twofold:
1. Update the Qt binaries used by the appveyor CI job after a recent update to Visual Studio 2019 used in the Appveyor build image resulted in ABI incompatibilities,
2. Remove optimisations and debug information from the Bitcoin Core `Release` msvc build to reduce the chance of future ABI incompatibility issues for future Visual Studio updates.
The changes made in this PR are:
- Changed appveyor config file hash to use a new version of Qt pre-compiled binaries built for Visual Studio 2019 v16.8.1.
- Adjusted msvc compiler and linker settings to remove optimisations and debug information generation to help avoid future ABI issues on Visual Studio updates.
- Tidied up debug and release configuration blocks in common project file to avoid duplication.
- Updated appveyor config to use latest Visual Studio 2019 image.
- Bumped vcpkg version to tag `2020.11-1` for binary caching feature*.
See #20392 for related discussion.
*Binary caching is a new [vcpkg feature](https://github.com/microsoft/vcpkg/blob/master/docs/specifications/binarycaching.md) that allows dependency caching. This PR is not using the feature but by updating the vcpkg version it means it can be optionally used by other contributors in their own Appveyor configs. By caching the vcpkg dependencies using this feature my build times are reduced by approx 10 minutes.
ACKs for top commit:
MarcoFalke:
Concept ACK 2c69381f3d
laanwj:
Concept ACK 2c69381f3d
Tree-SHA512: 372f5b8b3b5f7e56b78045f9fc06a22fd9f5366d06e99e2f1eaad6d741680da74d0e6371e7bc580cb41f4d6696b2101b950167309d33e98242578b458ace813a
c82d15b6d1 depends: Do not force Precompiled Headers (PCH) for building Qt on Linux (Hennadii Stepanov)
Pull request description:
On CentOS 8 (Cirrus CI job) the forced `-pch` option breaks Qt build.
Removing `-pch` option does not affect build time for other systems:
- master (e2ff5e7b35):
```
$ time make -j 9 -C depends/ qt
...
Caching qt...
make: Leaving directory '/home/hebasto/guix/GitHub/bitcoin/depends'
real 4m22,359s
user 18m3,719s
sys 1m24,769s
```
- this PR:
```
$ time make -j 9 -C depends/ qt
...
Caching qt...
make: Leaving directory '/home/hebasto/guix/GitHub/bitcoin/depends'
real 4m14,862s
user 18m3,355s
sys 1m24,506s
```
Qt docs: https://doc.qt.io/qt-5/qmake-precompiledheaders.htmlFixes#20423
ACKs for top commit:
MarcoFalke:
review ACK c82d15b6d1
Tree-SHA512: 0f2a3712e90de881d00f8e56c363edde33dd4f5c117df5744ab4e51d0a8146331de7236bc8329d68ddd91535cd853e68ee80ef4cceb6a909786abfd8881b01e8
This moves the fee_estimates file management to the CBlockPolicyEstimator
Flush() method.
Co-authored-by: John Newbery <john@johnnewbery.com>
Signed-off-by: Antoine Poinsot <darosior@protonmail.com>
This moves the CBlockPolicyEstimator to the NodeContext, which get rids
of two globals and allows us to conditionally create the
CBlockPolicyEstimator (and to remove a circular dep).
Signed-off-by: Antoine Poinsot <darosior@protonmail.com>
Check both failure cases:
- Use a glibc symbol from a version that is too new
- Use a symbol from a library that is not in the allowlist
And also check a conforming binary.
Adding a similar check for Windows PE can be done in a separate PR.
fad7be584f test: Fix intermittent p2p_finerprint issue (MarcoFalke)
Pull request description:
A single sync_with_ping can't be used to drop a block announcement, as the block might be sent *after* the ping has been responded to.
Fix that by waiting for the block.
ACKs for top commit:
theStack:
ACK fad7be584f
Tree-SHA512: d43ba9d07273486858f65a26326cc6637ef743bf7b400e5048ba7eac266fb1893283e6503dd49f179caa1abab2977315fb70ba9fba34be9a817a74259d8e4034
fa5c4f12f5 ci: Adjust cirrus ci task names (MarcoFalke)
Pull request description:
The task names are too long for GitHub to display them properly without truncation in the "checks-view". Fix that by using a new naming scheme:
* Native builds don't mention "x86_64 Linux", as it is redundant, they do mention the OS (bionic or focal) in the name suffix
* Cross builds mention the target in the prefix and the OS (always bionic) in the suffix
* the macos native build simply says "macos native"
ACKs for top commit:
practicalswift:
ACK fa5c4f12f5: patch looks correct!
hebasto:
ACK fa5c4f12f5, I have reviewed the code and it looks OK, I agree it can be merged.
RiccardoMasutti:
ACK fa5c4f1
Tree-SHA512: 856deb0577c97c70069ef1d369991addc49522135c0ad9e382218fd79ba3d55a95d6c601288dcef0510764b92fbd30a9d7de32b08dc5be55482deab14049b892
1e62350ca2 refactor: Improve use of explicit keyword (Fabian Jahr)
c502a6dbfb lint: Use c++17 std in cppcheck linter (Fabian Jahr)
Pull request description:
I found the `extended-lint-cppcheck` linter still uses `std=c++11` when reviewing #20471. The only difference in the output after this change is one line is missing:
```
src/script/descriptor.cpp:159:5: warning: Struct 'PubkeyProvider' has a constructor with 1 argument that is not explicit. [noExplicitConstructor]
```
After some digging, I am still not sure why this one is ignored with c++17 when 40 other`noExplicitConstructor` warnings were still appearing.
In the second commit, I fix these warnings, adding `explicit` where appropriate and adding fixes to ignore otherwise.
ACKs for top commit:
practicalswift:
cr ACK 1e62350ca2: patch looks correct!
MarcoFalke:
review ACK 1e62350ca2
Tree-SHA512: dff7b324429a57160e217cf38d9ddbb6e70c6cb3d3e3e0bd4013d88e07afc2292c3df94d0acf7122e9d486322821682ecf15c8f2724a78667764c05d47f89a12
Tidied up debug and release configuration blocks in common project file to avoid duplication.
Updated appveyor config to use latest Visual Studio 2019 image.
Changed appveyor config file hash to use a new version of Qt pre-compiled binaries built for Visual Studio 2019 v16.8.1.
Bumped vcpkg version to tag '2020.11-1' for binary caching feature.
See #20392 for related discussion.
2b356117e9 ci: no-longer exclude feature_block in TSAN job (fanquake)
Pull request description:
The TSAN job is now running on Cirrus.
Increase the allocated memory to the [maximum allowed](https://cirrus-ci.org/guide/linux/#linux-containers).
ACKs for top commit:
jonasschnelli:
utACK 2b356117e9 - checked the CI run and confirmed that the feature_block runs: https://cirrus-ci.com/task/6008403543719936?command=ci#L3249
MarcoFalke:
review ACK 2b356117e9
Tree-SHA512: b774995600361c74bc3267b566e12add66a4604bdf88f6e3f69669edbb8d7aff6f20fdbf0ef98187be4730ce4e18b1939bbcecd993a5c5c1ff40b237c7921b71
