The new time-machine commit is Guix v1.2.0 with a yet-unupstreamed patch
for NSIS.
A few important changes:
1. Guix switched back from using CPATH to C{,PLUS}_INCLUDE_PATH as the
way to indicate #include search paths.
2. GCC's library is now split into a separate output, whereas before it
was included in the default output. This means that our gcc toolchain
packages need to propagate that output.
3. A few package versions were bumped
b4511e2e2e log: Prefix log messages with function name if -logsourcelocations is set (practicalswift)
Pull request description:
Prefix log messages with function name if `-logfunctionnames` is set.
Yes, exactly like `-logthreadnames` but for function names instead of thread names :)
This is a small developer ergonomics improvement: I've found this to be a cheap/simple way to correlate log output and originating function.
For me it beats the ordinary cycle of 1.) try to figure out a regexp matching the static part of the dynamic log message, 2.) `git grep -E 'Using .* MiB out of .* requested for signature cache'`, 3.) `mcedit filename.cpp` (`openemacs filename.cpp` works too!) and 4.) search for log message and scroll up to find the function name :)
Without any logging parameters:
```
$ src/bitcoind -regtest
2020-08-25T03:29:04Z Using RdRand as an additional entropy source
2020-08-25T03:29:04Z Using 16 MiB out of 32/2 requested for signature cache, able to store 524288 elements
2020-08-25T03:29:04Z Using 16 MiB out of 32/2 requested for script execution cache, able to store 524288 elements
2020-08-25T03:29:04Z Loaded best chain: hashBestChain=0fff88f13cb7b2c71f2a335e3a4fc328bf5beb436012afca590b1a11466e22ff height=0 date=2011-02-02T23:16:42Z progress=1.000000
2020-08-25T03:29:04Z block tree size = 1
2020-08-25T03:29:04Z nBestHeight = 0
2020-08-25T03:29:04Z Imported mempool transactions from disk: 0 succeeded, 0 failed, 0 expired, 0 already there, 0 waiting for initial broadcast
2020-08-25T03:29:04Z 0 addresses found from DNS seeds
```
With `-logthreadnames` and `-logfunctionnames`:
```
$ src/bitcoind -regtest -logthreadnames -logfunctionnames
2020-08-25T03:29:04Z [init] [ReportHardwareRand] Using RdRand as an additional entropy source
2020-08-25T03:29:04Z [init] [InitSignatureCache] Using 16 MiB out of 32/2 requested for signature cache, able to store 524288 elements
2020-08-25T03:29:04Z [init] [InitScriptExecutionCache] Using 16 MiB out of 32/2 requested for script execution cache, able to store 524288 elements
2020-08-25T03:29:04Z [init] [LoadChainTip] Loaded best chain: hashBestChain=0fff88f13cb7b2c71f2a335e3a4fc328bf5beb436012afca590b1a11466e22ff height=0 date=2011-02-02T23:16:42Z progress=1.000000
2020-08-25T03:29:04Z [init] [AppInitMain] block tree size = 1
2020-08-25T03:29:04Z [init] [AppInitMain] nBestHeight = 0
2020-08-25T03:29:04Z [loadblk] [LoadMempool] Imported mempool transactions from disk: 0 succeeded, 0 failed, 0 expired, 0 already there, 0 waiting for initial broadcast
2020-08-25T03:29:04Z [dnsseed] [ThreadDNSAddressSeed] 0 addresses found from DNS seeds
```
ACKs for top commit:
laanwj:
Code review ACK b4511e2e2e
MarcoFalke:
review ACK b4511e2e2e🌃
Tree-SHA512: d100f5364630c323f31d275259864c597f7725e462d5f4bdedcc7033ea616d7fc0d16ef1b2af557e692f4deea73c6773ccfc681589e7bf6ba970b9ec169040c7
9f21ed4037 [test] Check user agent string from test framework connections (John Newbery)
9ce4c3c4c1 [test] Add P2P_SERVICES to p2p.py (John Newbery)
010542614d [test] Move MY_RELAY to p2p.py (John Newbery)
9b4054cb7a [test] Move MY_SUBVERSION to p2p.py (John Newbery)
7e158a6910 [test] Move MY_VERSION to p2p.py (John Newbery)
652311165c [test] Move MIN_VERSION_SUPPORTED to p2p.py (John Newbery)
Pull request description:
The messages.py module should contain code and helpers for
[de]serializing p2p messages. Specific usage of those messages should
be in p2p.py. This PR moves test framework specific constants to p2p.py.
It also changes the SUBVERSION constant to be a string instead of a bytes object. That means that it needs to be explicitly converted to a bytes object to serialize into a version message. Failing to do so would cause an easy-to-spot bug. This should avoid silent failures like the one solved in #20522.
ACKs for top commit:
laanwj:
Code review ACK 9f21ed4037
Tree-SHA512: 41d46575ac0ec36ad074d6c6a5b9cef50b05eeb8ddd8ed0a8f0d0c4617cc7b8baa6580af5b83a668230ce1ac27bf0e56914d0361a48b1b05fd75e2e60350eeaf
GetLocalAddrForPeer() is only called in one place. The checks inside that
function make more sense to be carried out be the caller:
- fSuccessfullyConnected is already checked at the top of
SendMessages(), so must be true when we call GetLocalAddrForPeer()
- fListen can go into the conditional before GetLocalAddrForPeer() is
called.
Gossiping addresses to peers is the responsibility of net processing.
Change AdvertiseLocal() in net to just return an (optional) address
for net processing to advertise. Update function name to reflect
new responsibility.
de6b389d5d tests: Test getaddressinfo parent_desc (Andrew Chow)
e4ac869a0a rpc: Add parent descriptor to getaddressinfo output (Andrew Chow)
bbe4a36152 wallet: Add GetDescriptorString to DescriptorScriptPubKeyMan (Andrew Chow)
9be1437c49 descriptors: Add ToNormalizedString and tests (Andrew Chow)
Pull request description:
Adds `parent_desc` field to the `getaddressinfo` RPC to export a public descriptor. Using the given address, `getaddressinfo` will look up which `DescriptorScriptPubKeyMan` can be used to produce that address. It will then return the descriptor for that `DescriptorScriptPubKeyMan` in the `parent_desc` field. The descriptor will be in a normalized form where the xpub at the last hardened step is derived so that the descriptor can be imported to other wallets. Tests are added to check that the correct descriptor is being returned for the wallet's addresses and that these descriptors can be imported and used in other wallets.
As part of this PR, a `ToNormalizedString` function is added to the descriptor classes. This really only has an effect on `BIP32PubkeyProvider`s that have hardened derivation steps. Tests are added to check that normalized descriptors are returned.
ACKs for top commit:
Sjors:
utACK de6b389d5d
S3RK:
Tested ACK de6b389
jonatack:
Tested ACK de6b389d5d modulo a few minor comments
fjahr:
Code review ACK de6b389d5d
meshcollider:
Tested ACK de6b389d5d
Tree-SHA512: a633e4a39f2abbd95afd7488484cfa66fdd2651dac59fe59f2b80a0940a2a4a13acf889c534a6948903d701484a2ba1218e3081feafe0b9a720dccfa9e43ca2b
84716b134e Add "index/blockfilterindex -> validation -> index/blockfilterindex" to expected circular dependencies (Jonas Schnelli)
ab3a0a2fb9 Add functional test for blockfilterindex in prune-mode (Jonas Schnelli)
c286a22f7b Add debug startup parameter -fastprune for more effective pruning tests (Jonas Schnelli)
5e112269c3 Avoid pruning below the blockfilterindex sync height (Jonas Schnelli)
00d57ff768 Avoid accessing nullpointer in BaseIndex::GetSummary() (Jonas Schnelli)
6abe9f5b11 Allow blockfilter in conjunction with prune (Jonas Schnelli)
Pull request description:
Maintaining the blockfilterindexes in prune mode is possible and may lead to efficient p2p based rescans of wallets (restore backups, import/sweep keys) beyond the prune height (rescans not part of that PR).
This PR allows running the blockfilterindex(es) in conjunction with pruning.
* Bitcoind/Qt will shutdown during startup when missing block data has been detected ([re]enable `-blockfilterindex` when we already have pruned)
* manual block pruning is disabled during blockfilterindex sync
* auto-pruning is delayed during blockfilterindex sync
ToDos:
* [x] Functional tests
ACKs for top commit:
fjahr:
Code review ACK 84716b1
ryanofsky:
Code review ACK 84716b134e. Only changes since last review were suggested new FindFilesToPrune argument and test.
benthecarman:
tACK 84716b134e
Tree-SHA512: 91d832c6c562c463f7ec7655c08956385413a99a896640b9737bda0183607fac530435d03d87c3c0e70c61ccdfe73fe8f3639bc7d26d33ca7e60925ebb97d77a
e829c9afbf refactor: replace sizeof(a)/sizeof(a[0]) by std::size (C++17) (Sebastian Falbesoner)
365539c846 refactor: init vectors via std::{begin,end} to avoid pointer arithmetic (Sebastian Falbesoner)
63d4ee1968 refactor: iterate arrays via C++11 range-based for loops if idx is not needed (Sebastian Falbesoner)
Pull request description:
This refactoring PR picks up the idea of #19626 and replaces all occurences of `sizeof(x)/sizeof(x[0])` (or `sizeof(x)/sizeof(*x)`, respectively) with the now-available C++17 [`std::size`](https://en.cppreference.com/w/cpp/iterator/size) (as [suggested by sipa](https://github.com/bitcoin/bitcoin/pull/19626#issuecomment-666487228)), making the macro `ARRAYLEN` obsolete.
As preparation for this, two other changes are done to eliminate `sizeof(x)/sizeof(x[0])` usage:
* all places where arrays are iterated via an index are changed to use C++11 range-based for loops If the index' only purpose is to access the array element (as [suggested by MarcoFalke](https://github.com/bitcoin/bitcoin/pull/19626#discussion_r463404541)).
* `std::vector` initializations are done via `std::begin` and `std::end` rather than using pointer arithmetic to calculate the end (also [suggested by MarcoFalke](https://github.com/bitcoin/bitcoin/pull/20429#discussion_r567418821)).
ACKs for top commit:
practicalswift:
cr ACK e829c9afbf75e930db6c3fe77a269b0af5e7a3ad: patch looks correct
fanquake:
ACK e829c9afbf
MarcoFalke:
review ACK e829c9afbf 🌩
Tree-SHA512: b01d32c04b9e04d562b7717cae00a651ec9a718645047a90761be6959e0cc2adbd67494e058fe894641076711bb09c3b47a047d0275c736f0b2218e1ce0d193d
Among the context menu actions for each transaction in the transactionview is the 'Edit Label' action.
While all other actions apply directly to the selected transaction, the 'Edit Label' action applies to the address of the selected transaction. This creates a confusing UX scenario where the outcome of the action is ambiguous. The action of Editing a Label should instead be reserved for the Send and Receive tabs.
This PR removes the 'Edit Label' action from the transactionview context menu. Since the 'Edit Label' action will no longer be utilized in the transactionview, the 'Edit Label' function logic is also removed.
9266f7497f util: Use std::chrono for time getters (MarcoFalke)
3c2e16be22 time: add runtime sanity check (Cory Fields)
Pull request description:
I have a followup that should remove the last of our `boost:posix_time` usage in `ParseISO8601DateTime`, but that will likely need more cross-platform testing/discussion, so have just split them up as this change is straight forward.
ACKs for top commit:
practicalswift:
Tested ACK 9266f7497f
laanwj:
Code review ACK 9266f7497f
Tree-SHA512: 5471a60e65e9fa8ef48320743ef637f1d162724e717e0f5509118e1e5732fc0844656a9c09d3d1300eb657dcc7a1e1e67305d8c9ef959c63be67393607dd4ceb
7bf04e358a build: remove mostly pointless BOOST_PROCESS macro (fanquake)
Pull request description:
Performing a series of link checks for a Boost component that is
header-only doesn't make much sense, and currently means we just have
another confusing Boost macro in our tree. I'm not sure why this was
originally done this way; maybe Sjors or luke-jr can elaborate (#15382 (929cda5470))?
The macro also has the side-effect of producing confusing error
messages. i.e in #20744, the CI is currently failing with:
```bash
checking for boostlib >= 1.58.0 (105800) lib path in "/tmp/cirrus-ci-build/depends/x86_64-pc-linux-gnu/lib"... yes
checking for boostlib >= 1.58.0 (105800)... yes
checking whether the Boost::Process library is available... yes
configure: error: Could not find a version of the Boost::Process library!
```
This isn't useful, given there is no such thing as a `Boost::Process` library.
This PR just removes the macro entirely, but maintains a `--with-boost-process`
(defaulting to off), flag to configure. Hopefully this will also be
removed, in favour of `--enable/disable-external-signer` if/when #16546
is merged.
ACKs for top commit:
laanwj:
ACK 7bf04e358a
Tree-SHA512: b270a0250f32df2078f986c165b8977967d8c06df80bf2773f3442f74b395a3bfa6544af1024d9b6524d90d47a0f6304194b3aced0e2ecb88e75916da945ccb6
882ce25132 cli: Treat high detail levels as the maximum in -netinfo (Wladimir J. van der Laan)
Pull request description:
I somehow often type `-netinfo 5` which gets treated as `-netinfo 0`, after this change it's `-netinfo 4` which seems more convenient behavior.
ACKs for top commit:
jonatack:
ACK 882ce25132
theStack:
Tested ACK 882ce25132
Tree-SHA512: 5ed13213d00940c81f70c5fe5092f5fcc78c0184e1cc83b6c58a7bf24cf0f634816ce28f468aac588a4d202a6a7c1b411c0690099f1a8bf1999e662de4afcccd
1363b6c27d [doc / util] Use comments to clarify time unit for int64_t type. (Amiti Uttarwar)
47a7a1687d [util] Introduce a SetMockTime that takes chrono time (Amiti Uttarwar)
df6a5fc1df [util] Change GetMockTime to return chrono type instead of int (Amiti Uttarwar)
a2d908e1da [test] Throw error instead of segfaulting in failure scenario (Amiti Uttarwar)
9a3bbe8fc5 [test] Introduce a unit test helper to create a valid mempool transaction. (Amiti Uttarwar)
Pull request description:
Some miscellaneous improvements that came up when working on #21061
- The first commit is a helper to make valid mempool transactions & submit via ATMP. Introducing in this PR, using in #21061.
- The second commit is a small improvement in `miner_tests.cpp` that uses `BOOST_REQUIRE_EQUAL` to properly terminate the program instead of segfaulting in the failure scenario where the blocks do not include the expected number of transactions.
- The third commit changes the function signature of `GetMockTime()` to return a chrono type.
- The fourth & fifth commit overload `SetMockTime` to also accept chrono type, and adds documentation to indicate that the `int64_t` function signature is deprecated.
ACKs for top commit:
vasild:
ACK 1363b6c27d
Tree-SHA512: c72574d73668ea04ee4c33858f8de68b368780f445e05afb569aaf8564093f8112259b3afe93cf6dc2ee12a1ab5af1130ac73c16416132c1ba2851c054a67d78
Add a check that new connections from the test framework to the
node have the correct user agent string. This makes bugs easier
to detect if the user agent string ever changes.
The messages.py module should contain code and helpers for
[de]serializing p2p messages. Specific usage of those messages should
be in p2p.py. Therefore specify the nServices value in the calling code,
not in the messages.py module.
messages.py is for message and primitive data structures. Specifics
about the test framework's p2p implementation should be in p2p.py.
Also rename to P2P_VERSION_RELAY. Also rename msg_version.nRelay to
relay. In Bitcoin Core, this is referred to as fRelay, since it's a
bool, so this field has always been misnamed.
The messages.py module should contain code and helpers for
[de]serializing p2p messages. Specific usage of those messages should
be in p2p.py. Therefore move MY_SUBVERSION to p2p.py.
Also rename to P2P_SUBVERSION.
The messages.py module should contain code and helpers for
[de]serializing p2p messages. Specific usage of those messages should
be in p2p.py. Therefore move MY_VERSION to p2p.py.
Also rename to P2P_VERSION to distinguish it from
other versioning used in Bitcoin/Bitcoin Core.
Also always set the nVersion field in CBlockLocator to 0 and ignore the
field in deserialized messages. The field is not currently used for
anything in Bitcoin Core.
The messages.py module should contain code and helpers for
[de]serializing p2p messages. Specific usage of those messages should
be in p2p.py. Therefore move MIN_VERSION_SUPPORTED to p2p.py.
Also rename to MIN_P2P_VERSION_SUPPORTED to distinguish it from
other versioning used in Bitcoin/Bitcoin Core.
fafddfadda scripted-diff: Remove shadowing lock annotations (MarcoFalke)
Pull request description:
Would be good to not redundantly copy the lock annotation from the class declaration to the member implementation. Otherwise it may not result in a compile failure if a new lock requirement is added to the member implementation, but not the class declaration.
ACKs for top commit:
amitiuttarwar:
ACK `fafddfadda`, confirmed that the annotations removed were all redundant. confirmed the claim of potential issue :)
hebasto:
ACK fafddfadda
jonatack:
Light utACK fafddfadda verified that the removed annotations in the definitions correspond to those in their respective declarations
Tree-SHA512: ea095c6d4e0bedd70d4e2d8a42b06cfd90c161ebfcaac13558c5dc065601a732e5f812f332104b7daa087aa57b8b0242b177799d22eef7628d77d4d87f443bf2
fd0be92cff doc: Add instructions on how to fuzz the P2P layer using Honggfuzz NetDriver (practicalswift)
Pull request description:
Add instructions on how to fuzz the P2P layer using [Honggfuzz NetDriver](http://blog.swiecki.net/2018/01/fuzzing-tcp-servers.html).
Honggfuzz NetDriver allows for very easy fuzzing of TCP servers such as Bitcoin Core without having to write any custom fuzzing harness. The `bitcoind` server process is largely fuzzed without modification.
This makes the fuzzing highly realistic: a bug reachable by the fuzzer is likely also remotely triggerable by an untrusted peer.
Top commit has no ACKs.
Tree-SHA512: 9e98cb30f00664c00c8ff9fd224ff9822bff3fd849652172df48dbaeade1dd1a5fc67ae53203f1966a1d4210671b35656009a2d8b84affccf3ddf1fd86124f6e
de85af5cce test: store subversion (user agent) as string in msg_version (Sebastian Falbesoner)
Pull request description:
It seems more natural to treat the "subversion" field (=user agent string, see [BIP 14](https://github.com/bitcoin/bips/blob/master/bip-0014.mediawiki#Proposal)) of a node as pure string rather than a bytestring within the test framework. This is also suggested with the naming prefix in `msg_version.strSubVer`: one probably wouldn't expect a field starting with "str" to be a bytestring that needs further decoding to be useful. This PR moves the encoding/decoding parts to the serialization/deserialization routines so that the user doesn't have to bother with that anymore.
Note that currently, in the master branch the `msg_version.strSubVer` is never read (only in `msg_version.__repr__`); However, one issue that is solved by this PR came up while testing #19509 (not merged yet): A decoding script for binary message capture files takes use of the functional test framework convert it into JSON format. Bytestrings will be convered to hexstrings, while pure strings will (surprise surprise) end up without modification in the file.
So without this patch, we get:
```
$ jq . out.json | grep -m5 strSubVer
"strSubVer": "2f5361746f7368693a32312e39392e302f"
"strSubVer": "2f5361746f7368693a302e32302e312f"
"strSubVer": "2f5361746f7368693a32312e39392e302f"
"strSubVer": "2f5361746f7368693a302e32302e312f"
"strSubVer": "2f5361746f7368693a32312e39392e302f"
```
After this patch:
```
$ jq . out2.json | grep -m5 strSubVer
"strSubVer": "/Satoshi:21.99.0/"
"strSubVer": "/Satoshi:0.20.1/"
"strSubVer": "/Satoshi:21.99.0/"
"strSubVer": "/Satoshi:0.20.1/"
"strSubVer": "/Satoshi:21.99.0/"
```
ACKs for top commit:
jnewbery:
utACK de85af5cce
Tree-SHA512: ff23642705c858e8387a625537dfec82e6b8a15da6d99b8d12152560e52d243ba17431b602b26f60996d897e00e3f37dcf8dc8a303ffb1d544df29a5937080f9
std::chrono::system_clock.time_since_epoch and time_t(0) are not guaranteed
to use the Unix epoch timestamp, but in practice they almost certainly will.
Any differing behavior will be assumed to be an error, unless certain
platforms prove to consistently deviate, at which point we'll cope with it
by adding offsets.
Do a quick runtime check to verify that
time_t(0) == std::chrono::system_clock's epoch time == unix epoch.
Co-authored-by: Anthony Towns <aj@erisian.com.au>
If Boost is not found via AX_BOOST_BASE, we don't actually stop
configuring, only a warning is emitted:
```bash
checking for boostlib >= 1.58.0 (105800)... configure: We could not detect the boost libraries (version MINIMUM_REQUIRED_BOOST or higher). If you have a staged boost library (still not installed) please specify $BOOST_ROOT in your environment and do not give a PATH to --with-boost option. If you are sure you have boost installed, then check your version number looking in <boost/version.hpp>. See http://randspringer.de/boost for more documentation.
```
Instead we would usually fail when one of the other
AX_BOOST_* macros fails to find a library. These macros are slowly being
removed, and in any case, it makes more sense to fail earlier if Boost
is missing.
If Boost is unavailable, the failure now looks like:
```bash
checking for boostlib >= 1.58.0 (105800)... configure: We could not detect the boost libraries (version 1.58.0 or higher). If you have a staged boost library (still not installed) please specify $BOOST_ROOT in your environment and do not give a PATH to --with-boost option. If you are sure you have boost installed, then check your version number looking in <boost/version.hpp>. See http://randspringer.de/boost for more documentation.
configure: error: Boost is not available!
```
Note that we now just pass the version into AX_BOOST_BASE, which fixes
it's display in the output (rather than MINIMUM_REQUIRED_BOOST).
This package is currently installed as a side-effect of installing our
other libboost-*-dev packages. However as those continue to dissapear,
it makes sense to install boost dev explicitly.
9cc8e30125 test: fix sign comparison warning in socket tests (fanquake)
Pull request description:
This fixes:
```bash
In file included from test/sock_tests.cpp:10:
In file included from /usr/local/include/boost/test/unit_test.hpp:18:
In file included from /usr/local/include/boost/test/test_tools.hpp:46:
/usr/local/include/boost/test/tools/old/impl.hpp:107:17: warning: comparison of integers of different signs: 'const long' and 'const unsigned long' [-Wsign-compare]
return left == right;
~~~~ ^ ~~~~~
```
which was introduced in #20788.
ACKs for top commit:
practicalswift:
cr ACK 9cc8e30125
vasild:
ACK 9cc8e30125
Tree-SHA512: 7069a4fde5cec01be03f8477fe396e53658f170efbf1d9ef3339d553bb90a2be9f4acd6b348127b14cd2f91426e0cd1fc35d2d3c9f201cf748c0cf50f47e46a5
Performing a series of link checks for a Boost component that is
header-only doesn't make much sense, and currently means we just have
another confusing Boost macro in our tree. I'm not sure why this was
originally done this way; maybe Sjors or luke-jr can elaborate
(#15382 (929cda5470))?
The macro also has the side-effect of producing confusing error
messages. i.e in #20744, the CI is currently failing with:
```bash
checking for boostlib >= 1.58.0 (105800) lib path in "/tmp/cirrus-ci-build/depends/x86_64-pc-linux-gnu/lib"... yes
checking for boostlib >= 1.58.0 (105800)... yes
checking whether the Boost::Process library is available... yes
configure: error: Could not find a version of the Boost::Process library!
```
This isn't useful, given there is no such thing as a `Boost::Process`
library.
This PR just removes the macro entirely, but maintains a `--with-boost-process`
(defaulting to off), flag to configure. Hopefully this will also be
removed, in favour of `--enable-disable-external-signer` if/when #16546
is merged.
Carl Dong