5da96210fc doc: release note for getpeerinfo last_block/last_transaction (Jon Atack)
cfef5a2c98 test: rpc_net.py logging and test naming improvements (Jon Atack)
21c57bacda test: getpeerinfo last_block and last_transaction tests (Jon Atack)
8a560a7d57 rpc: expose nLastBlockTime/TXTime as getpeerinfo last_block/transaction (Jon Atack)
02fbe3ae0b net: add nLastBlockTime/TXTime to CNodeStats, CNode::copyStats (Jon Atack)
Pull request description:
This PR adds inbound peer eviction criteria `nLastBlockTime` and `nLastTXTime` to `CNodeStats` and `CNode::copyStats`, which then allows exposing them in the next commit as `last_transaction` and `last_block` Unix Epoch Time fields in RPC `getpeerinfo`.
This may be useful for writing missing eviction tests. I'd also like to add `lasttx` and `lastblk` columns to the `-netinfo` dashboard as described in https://github.com/bitcoin/bitcoin/pull/19643#issuecomment-671093420.
Relevant discussion at the p2p irc meeting http://www.erisian.com.au/bitcoin-core-dev/log-2020-08-11.html#l-549:
```text
<jonatack> i was specifically trying to observe and figure out how to test https://github.com/bitcoin/bitcoin/issues/19500
<jonatack> which made me realise that i didn't know what was going on with my peer conns in enough detail
<jonatack> i'm running bitcoin locally with nLastBlockTime and nLastTXTime added to getpeerinfo for my peer connections dashboard
<jonatack> sipa: is there a good reason why that (eviction criteria) data is not exposed through getpeerinfo currently?
<sipa> jonatack: nope; i suspect just nobody ever added it
<jonatack> sipa: thanks. will propose.
```
The last commit is optional, but I think it would be good to have logging in `rpc_net.py`.
ACKs for top commit:
jnewbery:
Code review ACK 5da96210fc
theStack:
Code Review ACK 5da96210fc
darosior:
ACK 5da96210fc
Tree-SHA512: 2db164bc979c014837a676e890869a128beb7cf40114853239e7280f57e768bcb43bff6c1ea76a61556212135281863b5290b50ff9d24fce16c5b89b55d4cd70
daed542a12 [net_processing] Move ProcessMessage to PeerLogicValidation (John Newbery)
c556770b5e [net_processing] Change PeerLogicValidation to hold a connman reference (John Newbery)
Pull request description:
Rather than ProcessMessage() being a static function in net_processing.cpp, make it a private member function of PeerLogicValidation. This is the start of moving static functions and global variables into PeerLogicValidation to make it better encapsulated.
ACKs for top commit:
jonatack:
ACK daed542a12 code review and debug tested
promag:
Code review ACK daed542a12.
MarcoFalke:
re-ACK daed542a12, only change is removing second commit 🎴
theStack:
Code Review ACK daed542a12
Tree-SHA512: ddebf410d114d9ad5a9e536950018ff333a347c035d74fcc101fb4a3f20a281782c7eac2b7d1bd1c8f6bc7e59f5b5630fb52c2e1b4c32df454fa584673bd021e
366913e307 build: AX_BOOST_THREAD serial 33 (Igor Cota)
cf0681133a build: disable D-Bus on Android by default (Igor Cota)
Pull request description:
I've been trying to build for Android on different OSes/Gitian with varying success. Build system is quite the beast and sometimes it doesn't get it right. To make sure it does these three little tweaks make the Android build more robust:
- disable D-Bus (Android doesn't support it and has its own way to trigger notifications)
- don't flag `-lpthread` when linking Boost, [Bionic has built-in support](https://stackoverflow.com/questions/30801752/android-ndk-and-pthread)
- ~~add `-static-libstdc++` to linker flags. This avoids having to bundle `libc++_shared` with CLI apps, still necessary with `bitcoin-qt` though (thanks Sjors)~~
I think these are small and fairly straightforward so I put them all into this one PR.
ACKs for top commit:
fanquake:
ACK 366913e307
Tree-SHA512: 31465fd228a5877c20aa2a05f98242d4eeb328b9b35bd1a7a3dcfb1ef51379d84053a81ade5a65436ffc1bc8ccd21f11ed0539eb10e827d182c0c04394629af0
fa30d5282c doc: Remove label from good first issue template (MarcoFalke)
Pull request description:
Good first issues aren't that frequent that manually assigning the label is a problem, but this fixes the spam problem (e.g. https://twitter.com/GoodFirstIssues/status/1295455089491161088 )
ACKs for top commit:
jnewbery:
ACK fa30d5282c
Tree-SHA512: 59e7c707637cc328e2443c2b7e5d2c82ef151739ad5afb6cea1a60501318dc8c4c81c95591eed8172581ac99d43cf826dcdd547e096eff1038137853af67a975
15ae4a17c4 test/fuzz: add a seed corpus generation option to the test_runner (Antoine Poinsot)
Pull request description:
This adds a startup option to test/fuzz/test_runner.py which allows to generate seed corpus to the passed `seed_dir` instead of using them.
ACKs for top commit:
MarcoFalke:
ACK 15ae4a17c4
Tree-SHA512: f80ad58e48cc45272eace33dbf377848f31cbd6a25433786d50e9700f70185dff6513f71d885d0727ed57a2aa49163bfbdbc51a8091e99b4b1bae71e1504e6a5
4148f55dd0 docs: Correct description for getblockstats's txs field (Nadav Ivgi)
Pull request description:
It does count the coinbase transaction.
Refs #19766
ACKs for top commit:
MarcoFalke:
ACK 4148f55dd0
theStack:
ACK 4148f55dd0
Tree-SHA512: ccd420f19242efbbbecfe822c825363bc89e26618834de0d805f5cdb07461c8bdc6e077c61ea8cd0d40564a96c67d8a71c68175c8543bb849909d7ae375b2a92
5067c5acc3 [test] Add test for getblockheader verboseness (Torhte Butler)
Pull request description:
Improve test coverage by adding a test for getblockheader with verbose argument set to false.
ACKs for top commit:
theStack:
ACK 5067c5acc3
Tree-SHA512: e55593f1026a89dc7b796fa985b4cbcdb596e91d80d42dfb0660bda1692aaa35749ec29f9cd7032803f6225afb323f085df1ef6a9982de87be8e098f7253cdd5
8ed2f1ed78 Remove unused includes (Marcin Jachymiak)
cf095a53fc Move comment about BaseIndex::DB from TxIndex::DB (Marcin Jachymiak)
Pull request description:
Moves a comment about the `BaseIndex::DB` from the `TxIndex::DB` into the correct place. Originally part of https://github.com/bitcoin/bitcoin/pull/14053.
ACKs for top commit:
fanquake:
ACK 8ed2f1ed78
Tree-SHA512: cb4e2b916c7ab996961cc2e1d910bc4b8a1700eb32b70fc1657ca720117a7a84f7337fe5e4fb30e047aa92c31eaa976eaaa5cb8f861877f2ff6f4a59bb94f4e9
124e1ee134 doc: Add release notes for getindexinfo RPC (Fabian Jahr)
c447b09458 test: Add tests for getindexinfo RPC (Fabian Jahr)
667bc7a7f7 rpc: Add getindexinfo RPC (Fabian Jahr)
Pull request description:
As I was playing with indices a I was missing an RPC that gives information about the active indices in the node. I think this can be helpful for many users, especially since there are some new index candidates coming up (#14053, #18000) that can give a quick overview without the user having to parse the logs.
Feature summary:
- Adds new RPC `listindices` (placed in Util section)
- That RPC only lists the actively running indices
- For each index it gives the name, whether it is synced and up to which block height it is synced
ACKs for top commit:
laanwj:
Re-ACK 124e1ee134
jonatack:
Code review re-ACK 124e1ee per `git range-diff a57af89 47a5372 124e1ee` no change since my last re-ACK, rebase only
Tree-SHA512: 3b7174c87951e6457fef099f530337803906baf32fb64261410b8def2c0917853d6a1bf3059cd590b1cc1523608f8916dafb327a431d27ecbf8d7454406b5b35
ed5cd12869 test: Distinguish between nodes(bitcoind) and peers(mininodes) in p2p_leak.py (Dhruv Mehta)
f6f082b934 test: remove `CNodeNoVersionIdle` from p2p_leak.py (Dhruv Mehta)
45cf55ccac test: remove `CNodeNoVersionMisbehavior` from p2p_leak.py (Dhruv Mehta)
Pull request description:
- Removes `CNodeNoVersionMisbehavior` per recommendation at https://github.com/bitcoin/bitcoin/pull/19657#issuecomment-669926458
- Removes `CNodeNoVersionIdle` because it is similarly unnecessary
- As someone new to the codebase, I found it easier to understand it if `no_version_disconnect_node` tries to overwhelm the peer with any message that is not version/verack.
- Per recommendation at https://github.com/bitcoin/bitcoin/pull/19727#pullrequestreview-468093555, made a clear distinction between nodes(bitcoind) and peers(mininode interface implementations)
ACKs for top commit:
jnewbery:
tested ACK ed5cd12869
amitiuttarwar:
utACK ed5cd12869
Tree-SHA512: 310a24c91fd837e7f65177edb55fe6142fb3559fae7867c5cdd9c9a23b1a02202b935ca9a82633fa7649f3de2fa221f6da906a7b5e499fc20f7254085033757d
71e0f07e9c util: remove unused c-string variant of atoi64() (Sebastian Falbesoner)
Pull request description:
This is another micro-PR "removing old cruft with potentially sharp edges" (quote by practicalswift, see #19739). Gets rid of the c-string variant of the function `atoi64()`, which is only used in fuzzers and on one place with `wallet/wallet.h` (where it is originally a `std::string` anyways and uses `.c_str()` -- this method call can simply be removed.)
ACKs for top commit:
practicalswift:
ACK 71e0f07e9c -- diff looks correct
laanwj:
ACK 71e0f07e9c
Tree-SHA512: 4d1d28e2f5274fdbe0652e7a0f83dd416f4d19c1e1a49979927960a3ad40b0990eeaa4374656bf2c6998a965a14d62c1bc78303b7d583d3307c17828030a8e3b
356988e200 util: make EncodeBase58Check consume Spans (Sebastian Falbesoner)
f0fce0675d util: make EncodeBase58 consume Spans (Sebastian Falbesoner)
Pull request description:
This PR improves the interfaces for the functions `EncodeBase58{Check}` by using Spans, in a similar fashion to e.g. PRs #19660, #19687. Note that on the master branch there are currently two versions of `EncodeBase58`: one that takes two pointers (marking begin and end) and another one that takes a `std::vector<unsigned char>` const-ref. The PR branch only leaves one generic Span-interface, both simplifying the interface and allowing more generic containers to be passed. The same is done for `EncodeBase58Check`, where only one interface existed but it's more generic now (e.g. a std::array can be directly passed, as done in the benchmarks).
ACKs for top commit:
laanwj:
Code review ACK 356988e200
Tree-SHA512: 47cfccdd7f3a2d4694bb8785e6e5fd756daee04ce1652ee59a7822e7e833b4a441ae9362b9bd67ea020d2b5b7d927629c9addb6abaa9881d8564fd3b1257f512
fa0538e94d ci: Set cirrus RAM to 8GB (MarcoFalke)
fa41810d0e ci: Run valgrind fuzzer on cirrus (MarcoFalke)
Pull request description:
The first commit should fix the 50min timeout in forked repos. Similar to #19424. E.g. https://travis-ci.org/github/bitcoin-core/gui/builds/718322267
The second commit should fix#19744
Top commit has no ACKs.
Tree-SHA512: c765098dfa913ca49b1d1eee99aaa83e4b9eb191b7ad5e652e3f04744fe8670dd3ef4215832b8e2b5bac0273d24f607fc275e72f566326108ba42ab57228ffd4
It's also clearer to have `no_version_disconnect_node` send a message
other than version or verack in order to reach the peer discouragement
threshold.
72ae20fc14 tests: add sync_all to fix race condition in wallet groups test (Karl-Johan Alm)
Pull request description:
This most likely fixes#19749, the intermittent CI issues with wallet_groups.
This fix is also included in #19743.
Top commit has no ACKs.
Tree-SHA512: dd6ef7f89829483e2278191c21fe0912b51fd2187c10a0fa158339c5ab9f22d93b733ae10f17ef25d8b64f44e596e66dba8d7db5c009343472f422ce4cd67d8f
2f8a4c9a06 build: Enable some commonly enabled compiler diagnostics (practicalswift)
Pull request description:
Enable some commonly enabled compiler diagnostics as discussed in #17344.
| Compiler diagnostic | no# of emitted unique GCC warnings in `master` | no# of emitted unique Clang warnings in `master` |
| ------------- | ------------- | ------------- |
| `-Wduplicated-branches`: Warn if `if`/`else` branches have duplicated code | 0 | Not supported |
| `-Wduplicated-cond`: Warn if `if`/`else` chain has duplicated conditions | 0 | Not supported |
| `-Wlogical-op`: Warn about logical operations being used where bitwise were probably wanted | 0 | Not supported |
| `-Woverloaded-virtual`: Warn if you overload (not `override`) a virtual function | 0 | 0 |
| ~~`-Wunused-member-function`: Warn on unused member function~~ | Not supported | 2 |
| ~~`-Wunused-template`: Warn on unused template~~ | Not supported | 1 |
There is a large overlap between this list and [Jason Turner's list of recommended compiler diagnostics in the Collaborative Collection of C++ Best Practices (`cppbestpractices`) project](https://github.com/lefticus/cppbestpractices/blob/master/02-Use_the_Tools_Available.md#gcc--clang). There is also an overlap with the recommendations given in the [C++ Core Guidelines](https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines) (with editors Bjarne Stroustrup and Herb Sutter).
Closes#17344.
ACKs for top commit:
jonatack:
ACK 2f8a4c9a06 no warnings for me with these locally on debian 5.7.10-1 (2020-07-26) x86_64 with gcc 10 and clang 12
fanquake:
ACK 2f8a4c9a06 - no-longer seeing any obvious issues with doing this.
hebasto:
ACK 2f8a4c9a06, no new warnings in Travis jobs.
Tree-SHA512: f669ea22b31263a555f999eff6a9d65750662e95831b188c3192a2cf0127fb7b5136deb762a6b0b7bbdfb0dc6a40caf48251a62b164fffb81dd562bdd15ec3c8
4792cad88c doc: comment out and add annotation to unused MSG_FILTERED_WITNESS_BLOCK (Adam Jonas)
Pull request description:
Commenting out and adding a note to unused `MSG_FILTERED_WITNESS_BLOCK` [defined in BIP144](https://github.com/bitcoin/bips/blob/master/bip-0144.mediawiki#relay).
There was an attempt to make use of this in https://github.com/bitcoin/bitcoin/pull/10350, but it was closed due to lack of support. (h/t sdaftuar for pointing to the PR and jnewbery for the idea)
ACKs for top commit:
jnewbery:
Obvious ACK 4792cad88c
theStack:
ACK 4792cad88c📜
MarcoFalke:
cr ACK 4792cad88c good to keep it around in a comment to avoid accidental future re-assignment
practicalswift:
ACK 4792cad88c
Tree-SHA512: 22327ddded643ae50fdb529e4529a9b464f74e90620d0d2079a11070eaa8afe8363f6e14cca52f3bec2c9f87ee13e318edc6c5193761c94b8ae77be353a8da1f
fa55c1d5fd build: Add Werror=range-loop-analysis (MarcoFalke)
Pull request description:
The warning is implicitly enabled for Bitcoin Core. Also explicitly since commit d92204c900.
To avoid "fix range loop" follow-up refactors, we have two options:
* Disable the warning, so that issues never appear
* Enable it as an error, so that issues are either caught locally or by ci
ACKs for top commit:
fanquake:
ACK fa55c1d5fd
practicalswift:
ACK fa55c1d5fd -- pre-review fix-up is better than post-review fix-up
hebasto:
re-ACK fa55c1d5fd
Tree-SHA512: 019aa133f254af8882c1d5d10c420d9882305db0fc2aa9dad7d285168e2556306c3eedcc03bd30e63f11eae4cc82b648d83fb6e9179d6a6364651fb602d70134
7668db3b08 Move only: Move CDiskTxPos to its own file (Marcin Jachymiak)
Pull request description:
Moves `CDiskTxPos` it its own file so it can be used without the `txindex.h` include elsewhere. Originally part of #14053.
ACKs for top commit:
jnewbery:
utACK 7668db3b08
promag:
ACK 7668db3b08.
Tree-SHA512: b108e980ad04e43d1323410c3683a82bed70aee7795f5d8a2afbaf32a07ba598571f00b047bdde15048124b17178bcbd10654c48461beac988e9643cb2df664c
7f13dfb587 test: test the implicit avoid partial spends functionality (Karl-Johan Alm)
b82067bf69 wallet: try -avoidpartialspends mode and use its result if fees are below threshold (Karl-Johan Alm)
Pull request description:
The `-avoidpartialspends` feature is normally disabled, as it may affect the optimal fee for payments. This PR introduces a new parameter `-maxapsfee` (max avoid partial spends fee) which acts on the following values:
* -1: disable partial spend avoidance completely (do not even try it)
* 0: only do partial spend avoidance if fees are the same or better as the regular coin selection
* 1..∞: use APS variant if the absolute fee difference is less than or equal to the max APS fee
For values other than -1, the code will now try partial spend avoidance once, and if that gives a value within the accepted range, it will use that.
Example: -maxapsfee=0.00001000 means the wallet will do regular coin select, APS coin select, and then pick AKS iff the absolute fee difference is <= 1000 satoshi.
Edit: updated this to reflect the fact we are now using a max fee.
ACKs for top commit:
fjahr:
tested ACK 7f13dfb587
achow101:
ACK 7f13dfb587
jonatack:
ACK 7f13dfb58, code review, debug build, verified the test fails with `AssertionError: not(2 == 1)` for the number of vouts when `-maxapsfee=0.0001` is changed to 0, and verified the new logging with an added assertion.
meshcollider:
Code review ACK 7f13dfb587
Tree-SHA512: 475929df57f6191bb4e36bfbcad5a280a64bb0ecd8767b76cb2e44e2301235d0eb294a3f2fac5bbf15d35d7ecfba47acb2285feadb883c9ce31c08377e3afb3c
767073fb96 Shrink CAddress from 48 to 40 bytes on x64 (Vasil Dimov)
Pull request description:
`CAddress` inherits `CService` which is 28 bytes (on 64 bit machines).
`CAddress` then adds two member variables - one that requires 4 byte
alignment (`nTime`) and one that requires 8 byte alignment
(`nServices`).
Declare the smaller one first so that it fits in bytes 29..32.
On 32 bit machines this change has no effect and `CAddress` remains 40
bytes.
ACKs for top commit:
laanwj:
ACK 767073fb96
theStack:
ACK 767073fb96
Tree-SHA512: 73d6a4fcfa2687b4076950801871252e369510ecf09f820576dbeca9ee3ee94d14672e7d5596cb45fedd9e4b973dd0716a2ea3f13fc3058b4b697d036a7c9db0
9e7894357e test: speedup p2p_feefilter.py by whitelisting peers (immediate tx relay) (Sebastian Falbesoner)
fe3f0cc44e test: use wait_until for invs matching in p2p_feefilter.py (Sebastian Falbesoner)
6d941923c3 test: add logging for p2p_feefilter.py (Sebastian Falbesoner)
Pull request description:
This PR gives some love to the functional test `p2p_feefilter.py` by introducing the following changes:
* add missing log messages for the `test_feefilter` subtest (the main one)
* deduplicate code by using the utility function `wait_until` (already using the [recently introduced version](12410b1feb)) instead of a manual condition checking loop with `time.sleep`
* improve naming of the function `matchAllInvs` (more expressive name, snake_case) and moving it from global namespace to the connection class `FeefilterConn`
* speeding up the test significantly by the good ol' method of activating immediate tx relay (as seen on e.g. https://github.com/bitcoin/bitcoin/pull/17121, https://github.com/bitcoin/bitcoin/pull/17124, https://github.com/bitcoin/bitcoin/pull/17340, https://github.com/bitcoin/bitcoin/pull/17362, ...):
```
master branch:
$ time ./p2p_feefilter.py
...
real 0m39.367s
user 0m1.227s
sys 0m0.571s
PR branch:
$ time ./p2p_feefilter.py
...
real 0m9.386s
user 0m1.120s
sys 0m0.577s
```
ACKs for top commit:
instagibbs:
code review ACK 9e7894357e
jonatack:
re-ACK 9e78943 per `git range-diff 3ab2582 ea74a3c 9e78943`
Tree-SHA512: fe21c1c5413df9165fea916b5d5f609d3ba33e7b5c3364b38eb824fcc55d9e6abddf27116cbc0b325913d451a73c44542040fb916aec9c46f805c6e12f6f10cf
fa330ec2fe test: Remove confusing and broken use of wait_until global (MarcoFalke)
fa6583c30b ci: Set increased --timeout-factor by default (MarcoFalke)
Pull request description:
Assuming that tests don't have a logic error or race, setting a high timeout should not cause any issues. The tests will still pass just as fast in the fastest case, but it allows for some buffer in case of slow disks or otherwise starved ci machines.
Fixes#19729
ACKs for top commit:
hebasto:
ACK fa330ec2fe, I have reviewed the code, and it looks OK, I agree it can be merged.
Tree-SHA512: 3da80ee008c7b08bab5fdaf7804d57c79d6fed49a7d37b9c54fc89756659fcb9981fd10afc4d07bd90d93c1699fd410a0110a3cd34d016873759d114ce3cd82d
