The bug this fixes is two-part.
1.The fIsBareMultisigStd global is being reused by other tests,
i.e script_p2sh_tests(set), after being set to false.
2. The order our tests run in doesn't always? seem to be random,
which meant that the script_p2sh tests would only fail if they
were run in an order where transaction_tests ran first, mutating
the fIsBareMultisigStd global.
This doesn't seem to happen when running make check, but if you
run src/test/test_bitcoin and pass --random=99999, the failure
in script_p2sh:
test/script_p2sh_tests.cpp:200: error: in "script_p2sh_tests/set": txTo[1].IsStandard
will occur (on most systems).
The new test was introduced in 1bb5d517aa.
f41d589669 Document better -keypool as a look-ahead safety mechanism (Antoine Riard)
Pull request description:
If after a backup, an address is issued beyond the initial
keypool range and none of the addresses in this range
is seen onchain, if a wallet is restored from backup, even in
case of rescan, funds may be loss due to the look-ahead
buffer not being incremented and so restored wallet not detecting
onchain out-of-range address as derived from its seed.
This scenario is theoretically unavoidable due to the requirement
of the keypool to have a max size. However, given the default
keypool size, this is unlikely. Document better keypool size
implications to avoid user setting a too low value.
While reviewing #17681, it took me a while to figure out the safety implications of keypool, I find it would be better to document this a bit farther to avoid users shooting themselves in the foot. For further context & discussion, see https://github.com/bitcoin/bitcoin/pull/17681#issuecomment-563613452
ACKs for top commit:
ryanofsky:
Code review ACK f41d589669. Just "Warning:" prefix added since the last review
jonatack:
ACK f41d589669 code review and build/test. The added `Warning:` since last review is a good addition.
Tree-SHA512: d3d0ee88fcdfc5c8841a2bd4bada0e4eeb412a0dce5054e5fb023643c2fa57206a0f3efb06890c245528dc4431413ed2fd5645b9319d26245d044c490b7f0db0
In order to determine whether to download or process a relayed transaction, we
try to determine if we already have the transaction, either in the mempool, in
our recently rejected filter, in our orphan pool, or already confirmed in the
chain itself.
Prior to this commit, the heuristic for checking the chain is based on whether
there's an output corresponding to the 0- or 1-index vout in our coin cache.
While that is a quick check, it is very imprecise (say if those outputs were
already spent in a block) -- we can do better by just keeping a rolling bloom
filter of the transactions in recent blocks, which will capture the case of a
transaction which has been confirmed and then fully spent already.
To avoid relay problems for transactions which have been included in a recent
block but then reorged out of the chain, we clear the bloom filter whenever a
block is disconnected.
4de934b9b5 Convert compression.h to new serialization framework (Pieter Wuille)
ca34c5cba5 Add FORMATTER_METHODS, similar to SERIALIZE_METHODS, but for formatters (Pieter Wuille)
Pull request description:
This is the next piece of the puzzle from #10785. It includes:
* The `FORMATTER_METHODS` macro, similar to `SERIALIZE_METHODS`, for defining a formatter with a unified serialization/deserialization implementation.
* Updating `compression.h` to consist of 3 formatters, rather than old-style wrappers (`ScriptCompression`, `AmountCompression`, `TxOutCompression`).
ACKs for top commit:
laanwj:
code review ACK 4de934b9b5
ryanofsky:
Code review ACK 4de934b9b5. Only change since last review is removing REF usages
Tree-SHA512: d52ca21eb1ce87d9bc3c90d00c905bd4fada522759aaa144c02a58b4d738d5e8647c0558b8ce393c707f6e3c4d20bf93781a2dcc1e1dcbd276d9b5ffd0e02cd6
b6c3e84e87 doc: Improve fuzzing docs for macOS users (Fabian Jahr)
Pull request description:
Adds several helpful hints for macOS users trying to get fuzzers to run locally using AFL or libFuzzer. These are partly based on this comment https://github.com/bitcoin/bitcoin/issues/17657#issuecomment-562869600 and discussions in the review club for #17860. See: https://bitcoincore.reviews/17860.html
Based on the doc in the current state I could not compile fuzzers for AFL or libFuzzer. Using these hints, I can
- compile and run fuzzers with AFL
- compile but **not** run fuzzers with libFuzzer
Fuzzers compiled with libFuzzers may be running but don't produce any output. Looking for others to test this to see if it is an issue with my local system. Especially interesting if you have been running libFuzzer fuzzers successfully on macOS before.
Edit: Closes #17914
ACKs for top commit:
MarcoFalke:
ACK b6c3e84e87
Sjors:
ACK b6c3e84
fanquake:
ACK b6c3e84e87 - I think this has been nitpicked enough, and importantly the commands look better now.
Tree-SHA512: fdbacbcf10e9353a4ac3d22edf88663e33185ad2f244b986ff74c513de05f9fa62c4d8b17985d2f9288834c124b352cf52280627b5ff095735b411b12482e2ec
3c1bc40205 Add extra logging of asmap use and bucketing (Gleb Naumenko)
e4658aa8ea Return mapped AS in RPC call getpeerinfo (Gleb Naumenko)
ec45646de9 Integrate ASN bucketing in Addrman and add tests (Gleb Naumenko)
8feb4e4b66 Add asmap utility which queries a mapping (Gleb Naumenko)
Pull request description:
This PR attempts to solve the problem explained in #16599.
A particular attack which encouraged us to work on this issue is explained here [[Erebus Attack against Bitcoin Peer-to-Peer Network](https://erebus-attack.comp.nus.edu.sg/)] (by @muoitranduc)
Instead of relying on /16 prefix to diversify the connections every node creates, we would instead rely on the (ip -> ASN) mapping, if this mapping is provided.
A .map file can be created by every user independently based on a router dump, or provided along with the Bitcoin release. Currently we use the python scripts written by @sipa to create a .map file, which is no larger than 2MB (awesome!).
Here I suggest adding a field to peers.dat which would represent a hash of asmap file used while serializing addrman (or 0 for /16 prefix legacy approach).
In this case, every time the file is updated (or grouping method changed), all buckets will be re-computed.
I believe that alternative selective re-bucketing for only updated ranges would require substantial changes.
TODO:
- ~~more unit tests~~
- ~~find a way to test the code without including >1 MB mapping file in the repo.~~
- find a way to check that mapping file is not corrupted (checksum?)
- comments and separate tests for asmap.cpp
- make python code for .map generation public
- figure out asmap distribution (?)
~Interesting corner case: I’m using std::hash to compute a fingerprint of asmap, and std::hash returns size_t. I guess if a user updates the OS to 64-bit, then the hash of asap will change? Does it even matter?~
ACKs for top commit:
laanwj:
re-ACK 3c1bc40205
jamesob:
ACK 3c1bc40205 ([`jamesob/ackr/16702.3.naumenkogs.p2p_supplying_and_using`](https://github.com/jamesob/bitcoin/tree/ackr/16702.3.naumenkogs.p2p_supplying_and_using))
jonatack:
ACK 3c1bc40205
Tree-SHA512: e2dc6171188d5cdc2ab2c022fa49ed73a14a0acb8ae4c5ffa970172a0365942a249ad3d57e5fb134bc156a3492662c983f74bd21e78d316629dcadf71576800c
b35567fe0b test: only declare a main() when fuzzing with AFL (fanquake)
Pull request description:
This fixes fuzzing using [libFuzzer](https://llvm.org/docs/LibFuzzer.html) on macOS, which caused a few issues during the recent review club. macOS users could only fuzz using afl, or inside a VM.
It seems that the `__attribute__((weak))` marking is not quite enough to properly mark `main()` as weak on macOS. See Apples docs on [Frameworks and Weak Linking](https://developer.apple.com/library/archive/documentation/MacOSX/Conceptual/BPFrameworks/Concepts/WeakLinking.html#//apple_ref/doc/uid/20002378-107262-CJBJAEID).
Have tested fuzzing using libFuzzer and AFL with this patch.
ACKs for top commit:
MarcoFalke:
ACK b35567fe0b
fjahr:
ACK b35567f
Tree-SHA512: b881fdd98c7e1587fcf44debd31f5e7a52df938059ab91c41d0785077b3329b793e051a2bf2eee64488b9f6029d9288c911052ec23ab3ab8c0561a2be1682dae
e80317be5f refactor: Remove redundant conditional (Bushstar)
Pull request description:
Conditional check against fMaster is now redundant as it is already checked as true. This originally made sense as the outer conditional was:
f9cae832e6/src/checkqueue.h (L86)
Removal of fQuit happened in the commit below.
30ded3e3d8 (diff-88316c9aa9514c038c9304297e672da5)
ACKs for top commit:
theStack:
ACK e80317be5f
hebasto:
ACK e80317be5f, I have reviewed the code, and it looks OK, I agree it can be merged.
promag:
ACK e80317be5f.
emilengler:
re-ACK e80317be5f
practicalswift:
ACK e80317be5f
Empact:
ACK e80317be5f
Tree-SHA512: 136ea1d02e3d65100a8758730617ccede7864e08e8404e42e65d45d4bf95a3bfea2ab9895c6e8833abd654557d3efbba02b25297a2a5eefc36a11e97bbe9134f
deaa6dd144 psbt: check output index is within bounds before accessing (Andrew Chow)
f1ef7f0aa4 Don't calculate tx fees for PSBTs with invalid money values (Andrew Chow)
Pull request description:
Fixes #17149
Two classes of issues were found by the psbt fuzzer: values out of range and causing overflows, and prevout indexes being out of range. This PR fixes both.
When accessing a specific output using the index given in the tx, check that it is actually a possible output before trying to access the output.
When summing and checking amounts for `decodepsbt` and `analyzepsbt`, make sure that the values are actually valid money values.. Otherwise, stop summing and don't show the fee. For `analyzepsbt`, return that the next role is the Creator since the Creator needs to remake the transaction to be valid.
ACKs for top commit:
practicalswift:
ACK deaa6dd144 -- only change since last ACK was the addition of tests
gwillen:
tested ACK deaa6dd, would also like to see this merged!
Tree-SHA512: 06c36720bbb5a7ab1c29f7d15878bf9f0d3e5760c06bff479d412e1bf07bb3e0e9ab6cca820a4bfedaab71bfd7af813807e87cbcdf0af25cc3f66a53a06dbcfd
b3c4d9bac6 test: rename test suite name "tx_validationcache_tests" to match filename (Sebastian Falbesoner)
Pull request description:
Quoting `src/test/README.md`, '`Adding test cases`':
> "The file naming convention is `<source_filename>_tests.cpp`
> and such files should wrap their tests in a test suite
> called `<source_filename>_tests`."
Currently the unit test source file `txvalidationcache_tests.cpp` contains a unit test suite with the name `tx_validationcache_tests`, which is fixed by this PR. The following shell script shows that this is the only mismatch and for all other unit test source files the test suite names are correct:
```
#!/bin/bash
shopt -s globstar
for test_full_filename in **/*_tests.cpp; do
test_name_file=`basename $test_full_filename .cpp`
test_name_suite=`sed -n "s/^.*TEST_SUITE(\(.*_tests\).*$/\1/p" $test_full_filename`
if [ $test_name_file != $test_name_suite ]; then
echo "TestFilename: $test_name_file != TestSuitname: $test_name_suite"
fi
done
```
ACKs for top commit:
practicalswift:
ACK b3c4d9bac6 -- expected naming is better than unexpected naming :)
kristapsk:
ACK b3c4d9bac6
Tree-SHA512: 29d409b1eb22057ee2cc407508e2580d2bc03f412401df11b8ecf77be5ada6bda8f7d2cb5338c5e079490fa12242c1fd6230a09e47252c1b0d9fe535a828ca4c
88c83636d5 guix: Update documentation for time-machine (Carl Dong)
e6050884fd guix: Pin Guix using `guix time-machine` (Carl Dong)
Pull request description:
An alternative to #16519, pinning our version of Guix and eliminating a `guix pull` and changing the default Guix profile of builders.
I think this method might be superior, as it:
- Eliminates the possibility of future changes to the `guix environment` command line interface breaking our builds
- Eliminates the need to set up a separate channel repo
It is a more general pinning solution than #16519.
-----
The reason why I didn't originally propose this is because `guix time-machine` is a recent addition to Guix, only available since `f675f8dec73d02e319e607559ed2316c299ae8c7`
ACKs for top commit:
fanquake:
ACK 88c83636d5
Tree-SHA512: 85e03b0987ffa86da73e02801e1cd8b7622698d70c4ba4e60561611be1e9717d661c2811a59b3e137b1b8eef2d0ba37c313867d035ebc89c3bd06a23a078064a
Quoting src/test/README.md, 'Adding test cases':
"The file naming convention is `<source_filename>_tests.cpp`
and such files should wrap their tests in a test suite
called `<source_filename>_tests`."
Currently the unit test source file txvalidationcache_tests.cpp contains a unit
test suite with the name tx_validationcache_tests, which is fixed by this commit.
The following shell script shows that this is the only mismatch and for all other
unit test source files the test suite names are correct:
#!/bin/bash
shopt -s globstar
for test_full_filename in **/*_tests.cpp; do
test_name_file=`basename $test_full_filename .cpp`
test_name_suite=`sed -n "s/^.*TEST_SUITE(\(.*_tests\).*$/\1/p" $test_full_filename`
if [ $test_name_file != $test_name_suite ]; then
echo "TestFilename: $test_name_file != TestSuitname: $test_name_suite"
fi
done
4f7127d1e3 gui: Make Intro consistent with prune checkbox (Hennadii Stepanov)
4824a7d36c gui: Add Intro::UpdateFreeSpaceLabel() (Hennadii Stepanov)
daa3f3fa90 refactor: Add Intro::UpdatePruneLabels() (Hennadii Stepanov)
e4caa82a03 refactor: Replace static variable with data member (Hennadii Stepanov)
2bede28cd9 util: Add PruneGBtoMiB() function (Hennadii Stepanov)
e35e4b2ba0 util: Add PruneMiBtoGB() function (Hennadii Stepanov)
Pull request description:
On master (a6f6333ba2) and on 0.19.0.1 the intro dialog with prune enabled (checkbox "Discard blocks..." is checked) provides a user with wrong info about the required disk space:
Also the paragraph "If you have chosen to limit..." is missed.
---
With this PR when prune checkbox is toggled, the related text labels and the amount of required space shown are updated (previously they were only updated when the data directory was updated):
---
This PR is an alternative to #17035.
**ryanofsky**'s [suggestion](https://github.com/bitcoin/bitcoin/pull/17035#discussion_r337594268) also has been implemented.
ACKs for top commit:
emilengler:
ACK 4f7127d1e3
Sjors:
tACK 4f7127d1e3
ryanofsky:
Code review ACK 4f7127d1e3. It seems like there are a few visible changes here:
jonasschnelli:
utACK 4f7127d1e3
Tree-SHA512: fa0bbdcfafde97d7906cda066cbd4608b936a71cae1b4cda3ee3aa2eed3a9795f279f14c6b1b4997278e094db891c7d3bb695368ba0882347aa42165a86e5172
4c524f0aad Bugfix: GUI: Hide the HD/encrypt icons earlier so they get re-shown if another wallet is open (Luke Dashjr)
Pull request description:
To reproduce bug, open 2 wallets, and close 1. You end up left without the HD/encrypt icons, despite having a wallet open still.
This works because the icons are re-shown after we remove the current wallet (if there's another wallet still open).
ACKs for top commit:
promag:
Tested ACK 4c524f0aad.
jonasschnelli:
utACK 4c524f0aad
hebasto:
ACK 4c524f0aad, tested on Linux Mint 19.3.
Tree-SHA512: 4ef1bd4a0ae2f20ace9d02bc5d778640c11e46a86f30b762f8502e577f85114f0644d51a70cfbc4c23b51869c3caf20e94548aa64f51fdb85aea5f194a23fca6
44f15cfdcf gui: renamed 'debug window' to 'node window' (Zero)
Pull request description:
**Edit**: I have now limited the change in this PR to only renaming the window title from `Debug Window` to `Node Window`. Check [this comment](https://github.com/bitcoin/bitcoin/pull/17096#issuecomment-542837511) for more details.
This PR is in response to #17082, which aims to rename the `Debug window` title to a more user friendly term; `Node window`.
Closes #17082
ACKs for top commit:
hebasto:
ACK 44f15cfdcf, tested on Linux Mint 19.3:
theStack:
ACK 44f15cfdcf, tested on Linux (Lubuntu 16.04):
Tree-SHA512: 9fc73f2e67badb38525c550ce4c313288858b3fde30ef17fee85230be5bf31cf94408c699265b5e1256dfed60f8d04f48927d9b2831ba9f25498b98e6fa7180f
1a638e1105 gui: Shortcut to close ModalOverlay (Emil Engler)
Pull request description:
This adds the shortcut `Esc` to hide the ModalOverlay.
The motivation is that it is annoying to always move the cursor to "Hide" when quickly testing something in the GUI with an outdated chain.
ACKs for top commit:
kristapsk:
ACK 1a638e1105. Agree with @promag, Esc feels more natural than Enter here.
jonasschnelli:
ACK 1a638e1105
Tree-SHA512: ea764349ec145ce9a34cbc66c3ac0eace9233a3fb3e9c22694a77882478afa22d4e686ce2c1d7b3938f6769f96ba995577b0216ba9d98954dcf3e55d2187f2e0
2bcc70531a Updated appveyor job to checkout a specific vcpkg commit ID. (Aaron Clauson)
Pull request description:
This PR sets the vcpkg packages in stone by checking out a specific commit ID whenever they need to be reinstalled. The commit ID was chosen as the most recent commit at the time of this PR.
As per discussion on #17995 (and prior PR's/issues).
ACKs for top commit:
fanquake:
ACK 2bcc70531a - thanks for trying/suggesting all the different approaches, however this looks like the way to go. Should prevent `vcpkg` packages from changing out from under us.
Tree-SHA512: ced9c9c6df2287214a966d10110edda43a06380bae270a0d9ba1fd3dde48c27c109881423978e173b3e02512f6380600096b30510e90d37a6967fd8bf9186cb6
2d23082cbe bump test timeouts so that functional tests run in valgrind (Micky Yun Chan)
Pull request description:
ci/tests: Bump timeouts so all functional tests run on travis in valgrind #17763
Top commit has no ACKs.
Tree-SHA512: 5a8c6e2ea02b715facfcb58c761577be15ae58c45a61654beb98c2c2653361196c2eec521bcae4a9a1bab8e409d6807de771ef4c46d3d05996ae47a22d499d54
2525c096b0 build: remove configure checks for win libraries we don't link against (fanquake)
Pull request description:
While cross compiling, `HOST=x86_64-w64-mingw32`, none of these libs actually seem to be passed to the linker. i.e tailing a build with `make -j5 V=1 | rg -i 'mingwthrd|winspool|rpcrt4|crypt32'`.
I'm not 100% sure about `crypt32`, even though the majority of our Windows cryptography usage, i.e [`CryptAcquireContextW`](https://docs.microsoft.com/en-us/windows/win32/api/wincrypt/nf-wincrypt-cryptacquirecontextw) or [`CryptGenRandom`](https://docs.microsoft.com/en-us/windows/win32/api/wincrypt/nf-wincrypt-cryptgenrandom) is provided by `advapi32`.
Note that `rpcrt4` and `mingwthrd` are already missing from the MSVC build, so we can sync the remainder once it's clear what's actually needed. Hopefully sipsorcery can add some MSVC insight.
ACKs for top commit:
practicalswift:
ACK 2525c096b0 -- diff looks correct
sipsorcery:
ACK 2525c096b0.
Tree-SHA512: c756618f85ce2ab1e14e5514dbdc490d94c1c6dfd7a3e3d3b16344ae302fb789585dd10b5c2d784f961f3115bec1d914615051b3184bea00dfbcc3c23884ab4a
Needed for future ScriptPubKeyMans which may need to create
SigningProviders dynamically and thus a normal pointer is not enough
This commit does not change behavior.
Add wallet logic for dealing with multiple ScriptPubKeyMan instances. This
doesn't change current behavior because there is still only a single
LegacyScriptPubKeyMan. But in the future the new logic will be used to support
descriptor wallets.
