| Age | Commit message (Collapse) | Author |
|---|
|
|
|
|
|
location if -logsourcelocations is set
b4511e2e2ed1a6077ae6826a9ee6b7a311293d08 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 b4511e2e2ed1a6077ae6826a9ee6b7a311293d08
MarcoFalke:
review ACK b4511e2e2ed1a6077ae6826a9ee6b7a311293d08 🌃
Tree-SHA512: d100f5364630c323f31d275259864c597f7725e462d5f4bdedcc7033ea616d7fc0d16ef1b2af557e692f4deea73c6773ccfc681589e7bf6ba970b9ec169040c7
|
|
9f21ed4037758f407b536c0dd129f8da83173c79 [test] Check user agent string from test framework connections (John Newbery)
9ce4c3c4c1682032500c97af2d6383897b87c413 [test] Add P2P_SERVICES to p2p.py (John Newbery)
010542614dbebba5f5ad6a58c0554930e9e214fc [test] Move MY_RELAY to p2p.py (John Newbery)
9b4054cb7af22123c7fcc4989e143606a630b2af [test] Move MY_SUBVERSION to p2p.py (John Newbery)
7e158a69104831611462cb555da931331b237c78 [test] Move MY_VERSION to p2p.py (John Newbery)
652311165c4ef298dab71d7162f9054abf439f77 [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 9f21ed4037758f407b536c0dd129f8da83173c79
Tree-SHA512: 41d46575ac0ec36ad074d6c6a5b9cef50b05eeb8ddd8ed0a8f0d0c4617cc7b8baa6580af5b83a668230ce1ac27bf0e56914d0361a48b1b05fd75e2e60350eeaf
|
|
de6b389d5db7b8426313c5be6fbd290f992c5aa8 tests: Test getaddressinfo parent_desc (Andrew Chow)
e4ac869a0a0083e2e3af3b56301bd5c8e0cf650b rpc: Add parent descriptor to getaddressinfo output (Andrew Chow)
bbe4a36152fb8d9c8c3682ca2380f1c88cca61cb wallet: Add GetDescriptorString to DescriptorScriptPubKeyMan (Andrew Chow)
9be1437c49f986e8ed964d5f863b4bbcec340751 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 de6b389d5db7b8426313c5be6fbd290f992c5aa8
S3RK:
Tested ACK de6b389
jonatack:
Tested ACK de6b389d5db7b8426313c5be6fbd290f992c5aa8 modulo a few minor comments
fjahr:
Code review ACK de6b389d5db7b8426313c5be6fbd290f992c5aa8
meshcollider:
Tested ACK de6b389d5db7b8426313c5be6fbd290f992c5aa8
Tree-SHA512: a633e4a39f2abbd95afd7488484cfa66fdd2651dac59fe59f2b80a0940a2a4a13acf889c534a6948903d701484a2ba1218e3081feafe0b9a720dccfa9e43ca2b
|
|
84716b134e9afca2fba7731de4af1f22fa1b1518 Add "index/blockfilterindex -> validation -> index/blockfilterindex" to expected circular dependencies (Jonas Schnelli)
ab3a0a2fb915d8b8384c30a8b38b4b5cc35236fd Add functional test for blockfilterindex in prune-mode (Jonas Schnelli)
c286a22f7b63a8bd336d5d7606c339053ee0054b Add debug startup parameter -fastprune for more effective pruning tests (Jonas Schnelli)
5e112269c311a559bfded814d3c3c438349a1986 Avoid pruning below the blockfilterindex sync height (Jonas Schnelli)
00d57ff76854938ead800767fb673a8af46eac8e Avoid accessing nullpointer in BaseIndex::GetSummary() (Jonas Schnelli)
6abe9f5b11cd4a5ecb6caca8443fe2950a417842 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 84716b134e9afca2fba7731de4af1f22fa1b1518. Only changes since last review were suggested new FindFilesToPrune argument and test.
benthecarman:
tACK 84716b134e9afca2fba7731de4af1f22fa1b1518
Tree-SHA512: 91d832c6c562c463f7ec7655c08956385413a99a896640b9737bda0183607fac530435d03d87c3c0e70c61ccdfe73fe8f3639bc7d26d33ca7e60925ebb97d77a
|
|
e829c9afbf75e930db6c3fe77a269b0af5e7a3ad refactor: replace sizeof(a)/sizeof(a[0]) by std::size (C++17) (Sebastian Falbesoner)
365539c84691d470b44d35df374d8c049f8c1192 refactor: init vectors via std::{begin,end} to avoid pointer arithmetic (Sebastian Falbesoner)
63d4ee1968144cc3d115f92baef95785abf813ac 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 e829c9afbf75e930db6c3fe77a269b0af5e7a3ad
MarcoFalke:
review ACK e829c9afbf75e930db6c3fe77a269b0af5e7a3ad 🌩
Tree-SHA512: b01d32c04b9e04d562b7717cae00a651ec9a718645047a90761be6959e0cc2adbd67494e058fe894641076711bb09c3b47a047d0275c736f0b2218e1ce0d193d
|
|
9266f7497f256d780178829e0f3a29ddaeb794ba util: Use std::chrono for time getters (MarcoFalke)
3c2e16be22ae04bf56663ee5ec1554d0d569741b 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 9266f7497f256d780178829e0f3a29ddaeb794ba
laanwj:
Code review ACK 9266f7497f256d780178829e0f3a29ddaeb794ba
Tree-SHA512: 5471a60e65e9fa8ef48320743ef637f1d162724e717e0f5509118e1e5732fc0844656a9c09d3d1300eb657dcc7a1e1e67305d8c9ef959c63be67393607dd4ceb
|
|
7bf04e358a6550ac9851f1b2d87795927fc5ff4b 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 (929cda5470f98d1ef85c05b1cad4e2fb9227e3b0))?
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 7bf04e358a6550ac9851f1b2d87795927fc5ff4b
Tree-SHA512: b270a0250f32df2078f986c165b8977967d8c06df80bf2773f3442f74b395a3bfa6544af1024d9b6524d90d47a0f6304194b3aced0e2ecb88e75916da945ccb6
|
|
882ce25132e1b6880916fa154850a708e6a968b2 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 882ce25132e1b6880916fa154850a708e6a968b2
theStack:
Tested ACK 882ce25132e1b6880916fa154850a708e6a968b2
Tree-SHA512: 5ed13213d00940c81f70c5fe5092f5fcc78c0184e1cc83b6c58a7bf24cf0f634816ce28f468aac588a4d202a6a7c1b411c0690099f1a8bf1999e662de4afcccd
|
|
mempool transaction
1363b6c27dbd2614fd555d148ea624ed8b95f14e [doc / util] Use comments to clarify time unit for int64_t type. (Amiti Uttarwar)
47a7a1687d276bfa8769dee4bb78e8725f67a50e [util] Introduce a SetMockTime that takes chrono time (Amiti Uttarwar)
df6a5fc1dff3b1b7c2f2b67aad1ff17cac99f7b6 [util] Change GetMockTime to return chrono type instead of int (Amiti Uttarwar)
a2d908e1daa1d1be74568bd7d1d04b724da7d79c [test] Throw error instead of segfaulting in failure scenario (Amiti Uttarwar)
9a3bbe8fc57d88919acd4eadbc96124711f17ec2 [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 1363b6c27dbd2614fd555d148ea624ed8b95f14e
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.
|
|
fafddfadda0c77876ba764c5b65ee5fa8e53a5e0 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 fafddfadda0c77876ba764c5b65ee5fa8e53a5e0
jonatack:
Light utACK fafddfadda0c77876ba764c5b65ee5fa8e53a5e0 verified that the removed annotations in the definitions correspond to those in their respective declarations
Tree-SHA512: ea095c6d4e0bedd70d4e2d8a42b06cfd90c161ebfcaac13558c5dc065601a732e5f812f332104b7daa087aa57b8b0242b177799d22eef7628d77d4d87f443bf2
|
|
Honggfuzz NetDriver
fd0be92cff6a4b5e343e6ddae7481868354b9869 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
|
|
de85af5cce727981383ac0fe81f635451b331f23 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 de85af5cce727981383ac0fe81f635451b331f23
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>
|
|
901f54321b386258a1682423160bfdfa35ea4c39 guix: Passthrough BASE_CACHE into container (Carl Dong)
Pull request description:
This allows depends-built packages to be cached.
ACKs for top commit:
MarcoFalke:
Approach ACK 901f54321b386258a1682423160bfdfa35ea4c39
fanquake:
ACK 901f54321b386258a1682423160bfdfa35ea4c39
Tree-SHA512: 464815f41fc081d7956bec84380668834b6ee6751c7a3d56daad6e1fc91e582de4bbdd1a89f399b1136f2adc4d9941517cfe4db694f0ee5bf59bf2f44fc6fda0
|
|
9cc8e30125df14fe47e21e55ab3bf26f4d416565 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 9cc8e30125df14fe47e21e55ab3bf26f4d416565
vasild:
ACK 9cc8e30125df14fe47e21e55ab3bf26f4d416565
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 (929cda5470f98d1ef85c05b1cad4e2fb9227e3b0))?
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.
|
|
|
|
|
|
|
|
If the miner code is faulty and does not include any transactions in a block,
the code segfaults when it tries to access block transactions. Instead, add a
check that safely aborts the process.
|
|
|
|
1afc0e4aa1b910991d4f8a77d74e2197f370987c doc: remove potentially confusing ChainstateManager comment (James O'Beirne)
769a1ef9fdc9c372f5bbe91d1961cabd60bc1895 test: Add tests with maleated snapshot data (Fabian Jahr)
4d8de04f32736199e4b41a14a2d29b1a4d0a15d4 tests: add snapshot activation test (James O'Beirne)
31d225274ff1a4b245aea0a69f0e5224b0e64ca2 tests: add deterministic chain generation unittest fixture (James O'Beirne)
6606a4f8c616cf256537c3bfbdade9b43c51b4f5 move-onlyish: break out CreateUTXOSnapshot from dumptxoutset (James O'Beirne)
ad949ba449ff2115e3d22c71f5b6509f11112098 txdb: don't reset during in-memory cache resize (James O'Beirne)
f6e2da5fb7c6406c37612c838c998078ea8d2252 simplify ChainstateManager::SnapshotBlockhash() return semantics (James O'Beirne)
7a6c46b37edb8bfa0085d202aa7e9427d5e4fceb chainparams: add allowed assumeutxo values (James O'Beirne)
Pull request description:
This is part of the [assumeutxo project](https://github.com/bitcoin/bitcoin/projects/11):
Parent PR: #15606
Issue: #15605
Specification: https://github.com/jamesob/assumeutxo-docs/tree/master/proposal
---
This change proposes logic for activating UTXO snapshots, which is unused at the moment aside from an included unittest. There are a few moveonyish/refactoring commits to allow for halfway decent unittests.
Basic structure is included for specifying and checking the assumeutxo hash values used to validate activated snapshots. Initially I had specified a few height/hash pairs for mainnet in this change, but because of the security-critical nature of those parameters, I figured it was better to leave their inclusion to a future PR that includes only that change - my intent being that reviewers will be more likely to verify those parameters firsthand in a dedicated PR.
Aside from that and the snapshot activation logic, there are a few related changes:
- ~~allow caching the `nChainTx` value in the CCoinsViewDB; this is set during snapshot activation. Because we don't necessarily have access to the full chain at the time of snapshot load, this value is communicated through the snapshot metadata and must be cached within the chainstate to survive restarts.~~
- break out `CreateUTXOSnapshot()` from dumptxoutset. This is essentially a move-only change to allow the reuse of snapshot creation logic from within unittests.
- ...and a few other misc. changes that are solely related to unittests.
The move-onlyish commit is most easily reviewed with `--color-moved=zebra`.
ACKs for top commit:
fjahr:
Code review ACK 1afc0e4aa1b910991d4f8a77d74e2197f370987c
laanwj:
Code review ACK 1afc0e4aa1b910991d4f8a77d74e2197f370987c
Tree-SHA512: a4e4f0698f00a53ec298b5e8b7ef1c9fdf0185f95139d1b1f63cfdf6cbbd6d17b8c6e51bbf1de2e5f1a946bf49f8466232698ef55acce5a012c80b067da366ea
|
|
ef21fb7313005a8a2d4f03fb4056f1f66c1b04f0 zmq test: speedup test by whitelisting peers (immediate tx relay) (Sebastian Falbesoner)
5c6546362dce8b468268578e345c37ed515a1855 zmq test: fix flakiness by using more robust sync method (Sebastian Falbesoner)
8666033630eeaf851ec69e018bb53eb23093f4b9 zmq test: accept arbitrary sequence start number in ZMQSubscriber (Sebastian Falbesoner)
6014d6e1b5a0dda6e20c2721f0bdb7e6a63ece81 zmq test: dedup message reception handling in ZMQSubscriber (Sebastian Falbesoner)
Pull request description:
Fixes #20934 by using the "sync up" method described in https://github.com/bitcoin/bitcoin/issues/20538#issuecomment-738791868.
After improving robustness with this approach (commits 1-3), it turned out that there were still some fails, but those were unrelated to zmq: Out of 500 runs, 3 times `sync_mempool()` or `sync_blocks()` timed out, which can happen because the trickle relay time has no upper bound -- hence in rare cases, it takes longer than 60s. This is fixed by enabling immediate tx relay on node1 (commit 4), which as a nice side-effect also gives us a rough 2x speedup for the test.
For further details, also see the explanations in the commit messages.
There is no guarantee that the test is still not flaky, but it would help if potential reviewers would run the following script locally and report how many runs failed (feel free to do less than 1000 runs, as this takes quite a long if ran with `--valgrind`):
```
#!/bin/sh
OUTPUT_FILE=./zmq_results
echo ===== repeated zmq test ===== > $OUTPUT_FILE
for i in `seq 1000`; do
echo ------------------------
echo ----- test run $i -----
echo ------------------------
echo --- $i --- >> $OUTPUT_FILE
./test/functional/interface_zmq.py --valgrind
if [ $? -ne 0 ]; then
echo "FAILED. /o\\" >> $OUTPUT_FILE
else
echo "PASSED. \\o/" >> $OUTPUT_FILE
fi
done
echo Failed test runs:
grep FAILED $OUTPUT_FILE | wc -l
```
ACKs for top commit:
jonatack:
Light ACK ef21fb7313005a8a2d4f03fb4056f1f66c1b04f0 with the caveat that I was unable to make the test fail with valgrind both here and on master, so I can't vouch that it actually fixes the CI flakiness. The test does run ~2x faster with this.
Tree-SHA512: 7a1e7592fbbd98e69e1e1294486b91253e589c72b3c6bbb7f587028ec07cca59b7d984e4ebf256c4bc3e8a529ec77d31842f3dd874038aea0b684abfea50306a
|
|
This allows depends-built packages to be cached.
|
|
a5e15ae45ccae7948a6c5b95e6621f01afb88d55 scripted-diff: rename ping members (John Newbery)
45dcf2266125c65d7f546bdb211a278bd090a284 [net processing] Move ping data fields to net processing (John Newbery)
dd2646d12c172cb8899669af717c590483a17404 [net processing] Move ping timeout logic to net processing (John Newbery)
0b43b81f69ff13dbc1e893a80950f186690b4f62 [net processing] Move send ping message logic into function (John Newbery)
1a07600b4b0d08cffc7cd5c58af33fcd1ede558e [net] Add RunInactivityChecks() (John Newbery)
f8b3058992b507f3a6aac9d4e2db00102ae1b197 [net processing] Add Peer& arg to MaybeDiscourageAndDisconnect() (John Newbery)
Pull request description:
This continues the work of moving application layer data into net_processing, by moving all ping data into the new Peer object added in #19607.
For motivation, see #19398.
ACKs for top commit:
glozow:
reACK https://github.com/bitcoin/bitcoin/commit/a5e15ae45ccae7948a6c5b95e6621f01afb88d55
MarcoFalke:
review ACK a5e15ae45ccae7948a6c5b95e6621f01afb88d55 🥉
amitiuttarwar:
ACK a5e15ae45c
Tree-SHA512: fb84241613d6a6e1f2832fa5378030b5877a02e8308188f57ab545a6eaf2ab731a93abb7dcd3a7f7285bb66700f938096378a8e90cd6a3e6f3309f81d85a344e
|
|
expected circular dependencies
|
|
|
|
|
|
|
|
target
ffff84a9cb659562b3f560d3a489d4a62c71f793 fuzz: Remove expensive and redundant muhash from crypto fuzz target (MarcoFalke)
Pull request description:
Remove because it is redundant with `src/test/fuzz/muhash.cpp` and incredibly expensive
ACKs for top commit:
practicalswift:
Tested ACK ffff84a9cb659562b3f560d3a489d4a62c71f793
Tree-SHA512: c91ea2406db857127c789b9cdeb714a719d88b54132e9cef74fffd229532d874b6c043353793ec687504b5784afc74995f8982243d41f976b63d57454a5ed339
|
|
I somehow often type `-netinfo 5` which gets treated as `-netinfo 0`,
after this change it's `-netinfo 4` which seems more convenient behavior.
|
|
-BEGIN VERIFY SCRIPT-
sed -i 's/fPingQueued/m_ping_queued/g' src/net_processing.cpp
sed -i 's/nMinPingUsecTime/m_min_ping_time/g' src/net.* src/net_processing.cpp src/test/net_tests.cpp
sed -i 's/nPingNonceSent/m_ping_nonce_sent/g' src/net_processing.cpp
sed -i 's/nPingUsecTime/m_last_ping_time/g' src/net.*
-END VERIFY SCRIPT-
|
|
|
|
Ping messages are an application-level mechanism. Move timeout
logic from net to net processing.
|
|
|
|
Moves the logic to prevent running inactivity checks until
the peer has been connected for -peertimeout time into its
own function. This will be reused by net_processing later.
|
|
Refactor only. No change in behaviour.
|
|
automate helps
96635e61777add29b6a34d47767a63f43b2919af init: use GetNetworkNames() in -onlynet help (Jon Atack)
0dbde700a6e9894a8ead20f2eebd0ff6554ef2d9 rpc: use GetNetworkNames() in getnetworkinfo and getpeerinfo helps (Jon Atack)
1c3af37881adbbc37fb9924bcf69c403fcae1ae7 net: create GetNetworkNames() (Jon Atack)
b45eae4d5332f1da71ba9ec983fe7818fa4d32e9 net: update NET_UNROUTABLE to not_publicly_routable in GetNetworkName() (Jon Atack)
Pull request description:
per the IRC discussion today at http://www.erisian.com.au/bitcoin-core-dev/log-2021-01-19.html#l-87
- return a more helpful string name for `Network::NET_UNROUTABLE`: "not_publicly_routable" instead of "unroutable"
- update the RPC getpeerinfo "network" help, and automate it and the getnetworkinfo "network#name" and the -onlynet help doc generation
ACKs for top commit:
theStack:
re-ACK 96635e61777add29b6a34d47767a63f43b2919af 🌳
MarcoFalke:
review ACK 96635e61777add29b6a34d47767a63f43b2919af 🐗
Tree-SHA512: 511a7d987126b48a7a090739aa7c4964b6186a3ff8f5f7eec9233dd816c6b7a6dc91b3ea6b824aa68f218a8a3ebdc6ffd214e9a88af38f2bf23f3257c4284c3a
|
|
2ee4a7a9ec68c75094685c06ec793b614f44c4ce net: remove CNode::m_inbound_onion defaults for explicitness (Jon Atack)
24bda56c29800502953c6a8cd69248e60ff9a0a0 net: make CNode::m_inbound_onion public, drop getter, update tests (Jon Atack)
Pull request description:
Refactoring only, no change in behavior. This is a quick follow-up to #20210 to address these review comments:
- https://github.com/bitcoin/bitcoin/pull/20210#discussion_r528835313
- https://github.com/bitcoin/bitcoin/pull/20210#discussion_r550860416
- https://github.com/bitcoin/bitcoin/pull/20210#issuecomment-766093925
Changes:
- make the `CNode::m_inbound_onion class` member public, update the Doxygen comment, drop the getter, and update the tests
- remove the `CNode::m_inbound_onion` default value initialization in the ctor declaration and the member initializer in favor of always passing it explicitly to the ctor where we initialize it dynamically, to both clarify the caller code and to allow the compiler to warn if it is uninitialized in the ctor or omitted in the caller
ACKs for top commit:
MarcoFalke:
review ACK 2ee4a7a9ec68c75094685c06ec793b614f44c4ce 🏀
vasild:
ACK 2ee4a7a9ec68c75094685c06ec793b614f44c4ce
Tree-SHA512: 72961c91168885a9d881756b10bad9d587f5ce297d5a6493c23e267b7178ff22b697bc6868e7761d6304e372d2781453d30011a020afd506b1e308b4ffa5feee
|
|
3f8776a1391c3978ed66144df15fd9bcb9edd35d Re-add dead code detection (flack)
Pull request description:
This re-adds unreachable code detection for Python based on `vulture`.
Effectively, this reverts f4beb4996d27f2cdaf4f0a63e7dc044bf17decce. The difference to the previous version is that this runs with the `--min-confidence 100` setting. From https://pypi.org/project/vulture/:
> Use `--min-confidence 100` to only report code that is guaranteed to be unused within the analyzed files.
So this should avoid the previous issues where static analysis had wrong positives due to the dynamic nature of Python code by only reporting things that are unambiguous (such as code after a `return` statement). As such, there is not suppressions list.
My motivation was mainly #21081 which would have been caught by this (as can be seen by the CI run failing). This is still marked as draft because #21081 is needed to get the linter to pass. Also, there is a second problem that this found (see https://github.com/bitcoin/bitcoin/pull/19509/files#r571454691). From what I can tell, this is a spurious type comment that could just be removed (or if that line has no side effects it could also be deleted altogether?). I could add a commit here to fix it, but I wanted to see if there is interest in having this linter again in the first place
ACKs for top commit:
practicalswift:
ACK 3f8776a1391c3978ed66144df15fd9bcb9edd35d
Tree-SHA512: 52314ad4f627d969de1eb15375ca677ed86a2e816fe773756a1ce22421214ba407b5a09a4bf701a3aab1a10c7b336f548e4cef3327edf154acba55e987db21f6
|
|
|
