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
* Removed coinbasetxn from the getblocktemplate help outputs
* Added the missing name for transactions in the help outputs
* Added getblocktemplate help outputs for longpollid and default_witness_commitment
* Added more clarity to capabilities, rules, and coinbaseaux for getblocktemplate help (credit to luke-jr)
Co-authored-by: Luke Dashjr <luke+github_public@dashjr.org>
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
Onion addresses propagate very badly among the IPv4/IPv6 network, resulting
in difficulty for those to find each other.
The branching factor 1 is probably so low that propagations die out before
they reach another onion peer. Increase it to 1.5 on average.
7966aa424a Add variables for repeated scripts (MeshCollider)
fec8336ad9 Remove GetScriptForWitness function (MeshCollider)
b887060d06 Replace usage of GetScriptForWitness with GetScriptForDestination (MeshCollider)
Pull request description:
As per this TODO in the code:
> TODO: replace calls to GetScriptForWitness with GetScriptForDestination using the various witness-specific CTxDestination subtypes.
The commit "Add additional check for P2SH before adding extra wrapper" also adds an additional check that the scriptPubKey is a P2SH before auto-wrapping the witness script. We shouldn't wrap the witness script if not. Note: #16251 is even better than this check, please review that.
ACKs for top commit:
instagibbs:
ACK 7966aa424a
jonatack:
Code review re-ACK 7966aa4 per `git range-diff b4d0366 ed266f7 7966aa4`
achow101:
re-ACK 7966aa424a only changes since last is rebase.
Tree-SHA512: 3449e0e83bd842acc7c94544a85367da97ac20d859eefc1a618caef0c98204398c266fe8fb9600b78326df5175402e1ae4a132eb766e2c4485e7cda6a2a95c43
642ad31b41 Add loadwallet and createwallet RPC load_on_startup options (Russell Yanofsky)
Pull request description:
This maintains a persistent list of wallets stored in settings that will automatically be loaded on startup. Being able to load a wallet automatically on startup will be more useful in the GUI, but it's reasonable to expose this feature by RPC as well.
ACKs for top commit:
achow101:
re-ACK 642ad31b41 Only change is the test
meshcollider:
re-utACK 642ad31b41
Tree-SHA512: cca0b71bf1a83ad071830e6c459f1cd979b4add7144e899ec560da72b5910dd9bf9426e5c7d125ae96fad8990fbf81a76bc83c0459486c16086ada6cbde5eaa3
9adc2f80fc Refactor OutputGroups to handle effective values, fees, and filtering (Andrew Chow)
7d07e864b8 Use real value when calculating OutputGroup value (Andrew Chow)
Pull request description:
Currently, the effective values and filtering for positive effective values is done outside of the OutputGroup. We should instead have functions in Outputgroup to do this and call those for each OutputGroup. So this PR does that.
This makes future changes for effective values in coin selection much easier.
ACKs for top commit:
instagibbs:
reACK 9adc2f80fc
fjahr:
re-ACK 9adc2f80fc
meshcollider:
Light code review ACK 9adc2f80fc
Tree-SHA512: 7445c94b7295b45bcd83a6f8a5c8f6961a89453fcc856335192d4b4a66aec7724513616b04e5111588ab208c89b311055399d6279cd9c4ce452aefb85f04b64a
0e279fe489 walletdb: Remove unused static functions from walletdb.h (Andrew Chow)
9f536d4fe9 wallettool: Have RecoverDatabaseFile return errors and warnings (Andrew Chow)
06e263a4e3 Call RecoverDatabaseFile directly from wallettool (Andrew Chow)
Pull request description:
Followup to #19324 addressing some comments.
Removes the `SalvageWallet` function in wallettool and instead directly calls `RecoverDatabaseFile` as suggested in https://github.com/bitcoin/bitcoin/pull/19324#discussion_r450379596
Removes the `LogPrintf`s and `tfm::format`s in `RecoverDatabaseFile` as noted in https://github.com/bitcoin/bitcoin/pull/19324#discussion_r448027237
Removes the declarations of `VerifyEnvironment` and `VerifyDatabaseFile` that were forgotten in `walletdb.h` as noted in https://github.com/bitcoin/bitcoin/pull/19324#issuecomment-654389079
ACKs for top commit:
meshcollider:
Code review ACK 0e279fe489
ryanofsky:
Code review ACK 0e279fe489, just dropped last commit
Tree-SHA512: ffd01f30536c2eab4bf40ba363c3ea916ecef3c8f0c5262040b40498776ffb00f95240204a40e38415d6931800851d0a3fa63ee91efc1d329b60ac317da0363d
