3004d5a12d [validation] Remove fMissingInputs from AcceptToMemoryPool() (John Newbery)
c428622a5b [validation] Remove unused first_invalid parameter from ProcessNewBlockHeaders() (John Newbery)
7204c6434b [validation] Remove useless ret parameter from Invalid() (John Newbery)
1a37de4b31 [validation] Remove error() calls from Invalid() calls (John Newbery)
067981e492 [validation] Tidy Up ValidationResult class (John Newbery)
a27a2957ed [validation] Add CValidationState subclasses (John Newbery)
Pull request description:
Carries out some remaining tidy-ups remaining after PR 15141:
- split ValidationState into TxValidationState and BlockValidationState (commit from ajtowns)
- various minor code style tidy-ups to the ValidationState class
- remove the useless `ret` parameter from `ValidationState::Invalid()`
- remove the now unused `first_invalid` parameter from `ProcessNewBlockHeaders()`
- remove the `fMissingInputs` parameter from `AcceptToMemoryPool()`, and deal with missing inputs the same way as other errors by using the `TxValidationState` object.
Tip for reviewers (thanks ryanofsky!): The first commit ("[validation] Add CValidationState subclasses" ) is huge and can be easier to start reviewing if you revert the rote, mechanical changes:
Substitute the commit hash of commit "[validation] Add CValidationState subclasses" for <CommitHash> in the commands below.
```sh
git checkout <CommitHash>
git grep -l ValidationState | xargs sed -i 's/BlockValidationState\|TxValidationState/CValidationState/g'
git grep -l ValidationResult | xargs sed -i 's/BlockValidationResult\|TxValidationResult/ValidationInvalidReason/g'
git grep -l MaybePunish | xargs sed -i 's/MaybePunishNode\(ForBlock\|ForTx\)/MaybePunishNode/g'
git diff HEAD^
```
After that it's possible to easily see the mechanical changes with:
```sh
git log -p -n1 -U0 --word-diff-regex=. <CommitHash>
```
ACKs for top commit:
laanwj:
ACK 3004d5a12d
amitiuttarwar:
code review ACK 3004d5a12d. Also built & ran tests locally.
fjahr:
Code review ACK 3004d5a12d . Only nit style change and pure virtual destructor added since my last review.
ryanofsky:
Code review ACK 3004d5a12d. Just whitespace change and pure virtual destructor added since last review.
Tree-SHA512: 511de1fb380a18bec1944ea82b513b6192df632ee08bb16344a2df3c40811a88f3872f04df24bc93a41643c96c48f376a04551840fd804a961490d6c702c3d36
f201ba59ff Refactor: Split up CWallet and LegacyScriptPubKeyMan and classes (Andrew Chow)
6702048f91 MOVEONLY: Move key handling code out of wallet to keyman file (Andrew Chow)
ab053ec6d1 Move wallet enums to walletutil.h (Andrew Chow)
Pull request description:
Moves key management functions into a new class LegacyScriptPubKeyMan. First two commits are move-only commits which move stuff out of wallet.{h/cpp} and into newly created scriptpubkeyman.{h/cpp}. Third commit changes several things in CWallet to use LegacyScriptPubKeyMan.
First step in the wallet boxes refactor. Note that LegacyScriptPubKeyMan and ScriptPubKeyMan cannot be used standalone yet and are still very much tied into CWallet with both accessing functions within each other. This PR is to help reduce review burden.
ACKs for top commit:
Sjors:
Code review ACK f201ba5.
promag:
Code review ACK f201ba59ff.
ryanofsky:
Code review ACK f201ba59ff
MarcoFalke:
ACK f201ba59ff
Tree-SHA512: bdc0d8595a06233fe003afcf968a38e0e8cc584a6a89c5bcd05309ac29dca852391802d46763ef81a108d146d0f40c79ea5438e87234ed12b4b8360c9aec94c0
listsinceblock now checks that returned transactions are not
conflicting with any transactions that are filtered out by
the given blockhash
Co-Authored-By: Michael Chrostowski <michael.chrostowski@gmail.com>
ed2dc5e48a Add override/final modifiers to V1TransportDeserializer (Pieter Wuille)
f342a5e61a Make resetting implicit in TransportDeserializer::Read() (Pieter Wuille)
6a91499496 Remove oversized message detection from log and interface (Pieter Wuille)
b0e10ff4df Force CNetMessage::m_recv to use std::move (Jonas Schnelli)
efecb74677 Use adapter pattern for the network deserializer (Jonas Schnelli)
1a5c656c31 Remove transport protocol knowhow from CNetMessage / net processing (Jonas Schnelli)
6294ecdb8b Refactor: split network transport deserializing from message container (Jonas Schnelli)
Pull request description:
**This refactors the network message deserialization.**
* It transforms the `CNetMessage` into a transport protocol agnostic message container.
* A new class `TransportDeserializer` (unique pointer of `CNode`) is introduced, handling the network buffer reading and the decomposing to a `CNetMessage`
* **No behavioral changes** (in terms of disconnecting, punishing)
* Moves the checksum finalizing into the `SocketHandler` thread (finalizing was in `ProcessMessages` before)
The **optional last commit** makes the `TransportDeserializer` following an adapter pattern (polymorphic interface) to make it easier to later add a V2 transport protocol deserializer.
Intentionally not touching the sending part.
Pre-Requirement for BIP324 (v2 message transport protocol).
Replacement for #14046 and inspired by a [comment](https://github.com/bitcoin/bitcoin/pull/14046#issuecomment-431528330) from sipa
ACKs for top commit:
promag:
Code review ACK ed2dc5e48a.
marcinja:
Code review ACK ed2dc5e48a
ryanofsky:
Code review ACK ed2dc5e48a. 4 cleanup commits added since last review. Unaddressed comments:
ariard:
Code review and tested ACK ed2dc5e.
Tree-SHA512: bab8d87464e2e8742529e488ddcdc8650f0c2025c9130913df00a0b17ecdb9a525061cbbbd0de0251b76bf75a8edb72e3ad0dbf5b79e26f2ad05d61b4e4ded6d
faeb666536 util: Add CHECK_NONFATAL and use it in src/rpc (MarcoFalke)
Pull request description:
Fixes#17181
Currently, we use `assert` in RPC code to document logic and code assumptions. However, it seems a bit extreme to abort all of Bitcoin Core on an assert failure in one of the RPC threads. I suggest to replace all `assert`s with a macro `CHECK_NONFATAL(condition)` that throws a runtime error when the condition evaluates to `false`. That runtime error will then be returned to the rpc caller and will include instructions to report the error to our issue tracker.
ACKs for top commit:
practicalswift:
ACK faeb666536
laanwj:
ACK faeb666536
ryanofsky:
Code review ACK faeb666536
Tree-SHA512: 9b748715a5e0767ac11f1324a95a3a6ec672a0e0658013492219223bda83ce4b1b447fd8183bbb235f7df5ef7dddda7666ad569544b4d61cc65f232ca7a800ec
This moves CWallet members and methods dealing with keys to a new
LegacyScriptPubKeyMan class, and updates calling code to reference the new
class instead of CWallet.
Most of the changes are simple text replacements and variable substitutions
easily verified with:
git log -p -n1 -U0 --word-diff-regex=.
The only nontrivial chunk of code added is the new LegacyScriptPubKeyMan class
declaration, but this code isn't new and is just selectively copied and moved
from the previous CWallet class declaration. This can be verified with:
git log -p -n1 --color-moved=dimmed_zebra src/wallet/scriptpubkeyman.h src/wallet/wallet.h
or
git diff HEAD~1:src/wallet/wallet.h HEAD:src/wallet/scriptpubkeyman.h
This commit does not change behavior.
9075d13153 [docs] Add release notes for removal of REJECT reasons (John Newbery)
04a2f326ec [validation] Fix REJECT message comments (John Newbery)
e9d5a59e34 [validation] Remove REJECT code from CValidationState (John Newbery)
0053e16714 [logging] Don't log REJECT code when transaction is rejected (John Newbery)
a1a07cfe99 [validation] Fix peer punishment for bad blocks (John Newbery)
Pull request description:
We no longer send BIP 61 REJECT messages, so there's no need to set
a REJECT code in the CValidationState object.
Note that there is a minor bug fix in p2p behaviour here. Because the
call to `MaybePunishNode()` in `PeerLogicValidation::BlockChecked()` only
previously happened if the REJECT code was > 0 and < `REJECT_INTERNAL`,
then there are cases were `MaybePunishNode()` can get called where it
wasn't previously:
- when `AcceptBlockHeader()` fails with `CACHED_INVALID`.
- when `AcceptBlockHeader()` fails with `BLOCK_MISSING_PREV`.
Note that `BlockChecked()` cannot fail with an 'internal' reject code. The
only internal reject code was `REJECT_HIGHFEE`, which was only set in
ATMP.
This reverts a minor bug introduced in 5d08c9c579.
ACKs for top commit:
ariard:
ACK 9075d13, changes since last reviewed are splitting them in separate commits to ease understanding and fix nits
fjahr:
ACK 9075d13153, confirmed diff to last review was fixing nits in docs/comments.
ryanofsky:
Code review ACK 9075d13153. Only changes since last review are splitting the main commit and updating comments
Tree-SHA512: 58e8a1a4d4e6f156da5d29fb6ad6a62fc9c594bbfc6432b3252e962d0e9e10149bf3035185dc5320c46c09f3e49662bc2973ec759679c0f3412232087cb8a3a7
89339d1460 tests: Add test for loadblock option (Fabian Jahr)
Pull request description:
Fixes#17019
Was initially part of #17044 but as the test got larger it made sense to split it into its own commit as suggested in #17019 .
This is testing the `-loadblock` option by using the scripts in `contrib/linearize` to generate a `bootstrap.dat` file and starting a disconnected node with it. So it is also testing the linearize scripts which were untested before and needed to be made available for the CI environment, hence they are added to `DIST_CONTRIB` in `Makefile.am`.
ACKs for top commit:
laanwj:
ACK 89339d1460
Tree-SHA512: aede0cd6e8b21194973f3633bc07fa2672d66a6f85dfe6a57cee2bb269a65d19ea49d5f9ed7914a173b3847c76e70257aa865f44bde170c1999d9655b4862d1c
The use of native segwit addresses (pure p2wpkh instead of p2sh-p2wpkh) leads
to smaller transaction sizes, needing adaption of some constants in the
following test cases:
- test_dust_to_fee(): adaption of dust calculation
(p2wpkh spend estimate of 67 is taken from src/policy/policy.cpp:GetDustThreshold())
- test_maxtxfee_fails(): lowering -maxtxfee setting to trigger fail
facec1c643 wallet: Avoid showing GUI popups on RPC errors (MarcoFalke)
Pull request description:
RPC errors and warnings are shown as popups in the GUI instead of being returned to the RPC caller. For example,
```
$ ./src/bitcoin-cli loadwallet $(pwd)/./test/functional/data/wallets/high_minversion/
error code: -4
error message:
Wallet loading failed.
```
gives me a GUI popup and no reason why loading the wallet failed.
After this pull request:
```
$ ./src/bitcoin-cli loadwallet $(pwd)/./test/functional/data/wallets/high_minversion/
error code: -4
error message:
Wallet loading failed: Error loading /home/marco/workspace/btc_bitcoin_core/./test/functional/data/wallets/high_minversion/wallet.dat: Wallet requires newer version of Bitcoin Core
ACKs for top commit:
laanwj:
Code review ACK facec1c643
Tree-SHA512: c8274bbb02cfcf71676eeec1e773e51fb3538cf93f82e7cb8536f4716d44ed819cdc162dfc039ac7386a4db381a734cdb27fd32567043a1180c02519fbcba194
fa677d1801 ci: Remove redundant check for TRAVIS_OS_NAME (MarcoFalke)
fadccb263b doc: Document that GNU tools are required for linters (MarcoFalke)
4444704ca9 ci: Cleanup macOS runs (MarcoFalke)
Pull request description:
* Remove a commented out cleanup task in `before_cache`
* Remove the linter run on macOS, and document that GNU tools are required to run the linters
ACKs for top commit:
Sjors:
Code review ACK fa677d1801
laanwj:
ACK fa677d1801
ryanofsky:
Code review ACK fa677d1801 for new third commit replacing TRAVIS_OS_NAME check with NO_DEPENDS setting
Tree-SHA512: 9122a63bbe7887d9e379123152ea4ba44324cb18033b9e6b45bfdb1af665c10ea598564b9fcd57330d208a08e4696e41b4d6175f05f0843a3a76530da114f8c6
9576614d2d doc: Describe log files + consistent paths in test READMEs (Martin Erlandsson)
Pull request description:
picks up #15830
I saw this was almost ready to merge but the test logging part was not 100% correct. I reworked that part, the rest is the same.
ACKs for top commit:
GChuf:
ACK 9576614d2d
Tree-SHA512: 3de7f1b0a1b0419df6e7b55964d00e715b6cb7874b1849ad6f120597610d7df4182c4b61b9c9691ce04f4e392ed3caead4c623374be2066ac31319e702d45d09
The function implementing segwit v0 signature hash was originally named
SegwitVersion1SignatureHash() (presumably before segwit v0 was named
segwit v0). Rename it to SegwitV0SignatureHash().
Also rename SignatureHash() to LegacySignatureHash() for disambiguation.
fba4baa4fa test: speed up wallet_address_types by whitelisting peers (immediate tx relay) (Sebastian Falbesoner)
Pull request description:
approaches another part of #16613 ("Functional test suite bottlenecks")
As for `wallet_backup.py` (Commit 581c9be0d8), the
bottleneck is in relaying transactions. By whitelisting the peers, the
inventory is transmissioned immediately rather than on average every 5 seconds,
speeding up the test significantly:
before:
```
$ time ./wallet_address_types.py
real 1m30.072s
user 0m6.478s
sys 0m2.298s
```
with this PR:
```
$ time ./wallet_address_types.py
real 0m26.785s
user 0m5.525s
sys 0m1.888s
```
ACKs for top commit:
fanquake:
ACK - fba4baa4fa
Tree-SHA512: 6728ae44bd8839426fa943d06af884e40c2d88de5d7807269a1e78ff987077160aa7e8d395f4468e6ca1d6f2110c7a03cd346a3339b256702f4cdabd285f7f86
32d665c265 test: fix "tx-size-small" errors after default address change (Sebastian Falbesoner)
Pull request description:
Addresses #17043, affects RBF and BIP68 functional tests.
The "tx-size-small" policy rule rejects transactions with a non-witness size of
smaller than 82 bytes (see `src/validation.cpp:MemPoolAccept::PreChecks(...)`),
which corresponds to a transaction with 1 segwit input and 1 P2WPKH output.
Through the default address change, the created test transactions have segwit
inputs now and sending to short scriptPubKeys might violate this rule. By
bumping the dummy scriptPubKey size to 22 bytes (= the size of a P2WPKH
scriptPubKey), on all occurences the problem is solved.
The dummy scriptPubKey has the format:
```21 <21-byte-long string of 'a' or 1s>```
ACKs for top commit:
instagibbs:
reACK 32d665c265 just s/Bytes/bytes/
MarcoFalke:
ACK 32d665c265
Tree-SHA512: 80e0386ff3c3f462901ba5c1e5ef2cbf095d9c0a40c8c3cfeacd4a3ab676afe744aa95b9eed77b4b3eec88bed930b33aa718117ed0977f6374e858a2f3bd5c57
Addresses #17043, affects RBF and BIP68 functional tests.
The "tx-size-small" policy rule rejects transactions with a non-witness size of
smaller than 82 bytes (see src/validation.cpp:MemPoolAccept::PreChecks(...)),
which corresponds to a transaction with 1 segwit input and 1 P2WPKH output.
Through the default address change, the created test transactions have segwit
inputs now and sending to short scriptPubKeys might violate this rule. By
bumping the dummy scriptPubKey size to 22 bytes (= the size of a P2WPKH
scriptPubKey), on all occurences the problem is solved.
The dummy scriptPubKey has the format:
21 <21-byte-long string of 'a' or 1s>
former commit messages, now squashed:
test: rbf, bip68: use constant DUMMY_P2WPKH_SCRIPT for bumped scriptPubKey
test: rbf, bip68: use constant DUMMY_P2WPKH_SCRIPT for dummy scriptPubKeys (b'a' * 35)
test: rbf, bip68: comment DUMMY_P2WPKH_SCRIPT constant, put into common (new) module
approaches another part of #16613 ("Functional test suite bottlenecks")
As for wallet_backup.py (Commit 581c9be0d8), the
bottleneck is in relaying transactions. By whitelisting the peers, the
inventory is transmissioned immediately rather than on average every 5 seconds,
speeding up the test significantly:
before:
$ time ./wallet_address_types.py
real 1m30.072s
user 0m6.478s
sys 0m2.298s
with this PR:
$ time ./wallet_address_types.py
real 0m26.785s
user 0m5.525s
sys 0m1.888s
approaches part of #16613 ("Functional test suite bottlenecks")
The majority of the test time is spent in sync_mempools() after sending to
addresses, i.e. the bottleneck is in relaying transactions. By whitelisting the
peers via -whitelist, the inventory is transmissioned immediately rather than
on average every 5 seconds, speeding up the test by at least a factor of two:
before:
$ time ./wallet_backup.py
real 2m2.523s
user 0m6.093s
sys 0m2.454s
with this PR:
$ time ./wallet_backup_with_whitelist.py
real 0m36.570s
user 0m5.365s
sys 0m1.696s
Note that the test is not deterministic (the sendtoaddress RPC in function
one_send() is executed with a probability of 50%), hence the times could vary
between individual runs.
d478a472eb test: Fix combine_logs.py for AppVeyor build (Martin Zumsande)
Pull request description:
Fixes#16894
This fixes the problem of AppVeyor builds not showing `debug.log` if a functional test fails, because the windows separator `\` doesn't work together with the regex in `combine_logs.py`.
A fix was already attempted in #16896, however, that PR became inactive and was marked "up for grabs", plus it's a really small change.
As suggested by jamesob, this PR uses `pathlib`: For the glob and to convert the path to a posix-style string, it leaves the regex as is (in contrast to #16896 which adjusted the regex).
I tested this locally on Windows and Ubuntu.
Top commit has no ACKs.
Tree-SHA512: 603b4359b6009b6da874c30f69759acda03730ee5747898a0fe957a5fc37ee9ba07858c6aa2169bf4c40521f37e47138e8314d698652ea2760fa0a3f76b890bd
Remove the BIP61 REJECT code from error messages and logs when a
transaction is rejected.
BIP61 support was removed from Bitcoin Core in
fa25f43ac5. The REJECT codes will be
removed from the codebase entirely in the following commit.
9743432034 Fix bug where duplicate PSBT keys are accepted (John L. Jegutanis)
Pull request description:
As per the BIP 174 spec a PSBT key cannot be duplicated,
however the current code accepts key duplication.
The PSBT key/value entries can be duplicated when the value
is `empty()` or `IsNull()` for `CScript` or `CTxOut` respectively
and if those key/value entries are serialized before the non-empty ones.
For example, the following PSBT, included in the test vectors,
contains a duplicate field:
```
// magic
70736274ff
// global tx
//// key
0100
//// value
2a02000000000140420f000000000017a9146e91b72d5593e7d4391e2ff44e91e985c31641f08700000000
//// separator
00
// no inputs
// outputs
//// key PSBT_OUT_WITNESSSCRIPT
0101
//// value (empty script)
00
//// key PSBT_OUT_WITNESSSCRIPT (same as the above)
0101
//// value (an OP_RETURN script)
016a
//// separator
00
```
ACKs for top commit:
achow101:
ACK 9743432034
instagibbs:
code review ACK 9743432034
Tree-SHA512: 34f4b34c8e6561c6a6ab745cdd319f6687eac6f7cecc735c94035eeca8c5157e17a27f2ae853dbaa6634fcd5a8f4e1c6cc13d1ebd7e563459665d72bb147cc1e
fa25f43ac5 p2p: Remove BIP61 reject messages (MarcoFalke)
Pull request description:
Reject messages (BIP 61) appear in the following settings:
* Parsing of reject messages (in case `-debug=net` is set, off by default). This has only been used for a single `LogPrint` call for several releases now. Such logging is completely meaningless to us and should thus be removed.
* The sending of reject messages (in case `-enablebip61` is set, off by default). This can be used to debug a node that is under our control. Instead of hacking this debugging into the p2p protocol, it could be more easily achieved by parsing the debug log. (Use `-printtoconsole` to have it as stream, or read from the `debug.log` file like our python function `assert_debug_log` in the test framework does)
Having to maintain all of this logic and code to accommodate debugging, which can be achieved by other means a lot easier, is a burden. It makes review on net processing changes a lot harder, since the reject message logic has to be carried around without introducing any errors or DOS vectors.
ACKs for top commit:
jnewbery:
utACK fa25f43ac5
laanwj:
I'm still not 100% convinced that I like getting rid of BIP61 conceptually, but apparently everyone wants it, code review ACK fa25f43ac5.
ryanofsky:
Code review ACK fa25f43ac5
Tree-SHA512: daf55254202925e56be3d6cfb3c1c804e7a82cecb1dd1e5bd7b472bae989fd68ac4f21ec53fc46751353056fd645f7f877bebcb0b40920257991423a3d99e0be
4bb660be90 Add release note (Andrew Chow)
ed96b295d7 Update descriptors.md to include sortedmulti (Andrew Chow)
80be78ea75 Test sortedmulti descriptor using BIP 67 tests (Andrew Chow)
6f588fd227 Add sortedmulti descriptor and unit tests (Andrew Chow)
Pull request description:
Adds a `sortedmulti()` descriptor as mentioned in https://github.com/bitcoin/bitcoin/pull/17023#issuecomment-537596416.
`sortedmulti()` works in the same way as `multi` does but sorts the pubkeys in the resulting scripts in lexicographic order as described in [BIP67](https://github.com/bitcoin/bips/blob/master/bip-0067.mediawiki). Note that this does not add support for BIP67 nor is BIP67 fully supported by this descriptor (which is why it is not named `multi67()`) as it does not require compressed pubkeys.
Tests from BIP67 were added and documentation was updated.
ACKs for top commit:
instagibbs:
re-ACK 4bb660be90
Sjors:
re-ACK 4bb660be90
Tree-SHA512: 93b21112a74ebe0bf316d8f3e0291f69fd975cf0a29332f9728e7b880cad312b8b14007e86adcd7899f117b9303cbcf4cb35f3bb2f2f648d1a446f83f75a70a5
8acd58927a Fix Python Docstring to include all Args. (John Bampton)
Pull request description:
Found a Python function that had incorrect and missing arguments in its Docstring.
ACKs for top commit:
laanwj:
ACK 8acd58927a
Tree-SHA512: 936f275f29a700d630bb479b5283e47b66f2df76d8b8c053f594e6aedf783cc98a29c924c3a46613f112dfc884acb50f21a0b18f96d939e887b12b921ef2e10f
As per the BIP 174 spec a PSBT key cannot be duplicated,
however the current code accepts key duplication.
The PSBT key/value entries can be duplicated when the value
is `empty()` or `IsNull()` for `CScript` or `CTxOut` respectively
and if those key/value entries are serialized before the non-empty ones.
For example, the following PSBT, included in the test vectors,
contains a duplicate field:
```
// magic
70736274ff
// global tx
//// key
0100
//// value
2a02000000000140420f000000000017a9146e91b72d5593e7d4391e2ff44e91e985c31641f08700000000
//// separator
00
// no inputs
// outputs
//// key PSBT_OUT_WITNESSSCRIPT
0101
//// value (empty script)
00
//// key PSBT_OUT_WITNESSSCRIPT (same as the above)
0101
//// value (an OP_RETURN script)
016a
//// separator
00
```
ea4cc3a7b3 Truly decouple wallet from chainparams for -fallbackfee (Jorge Timón)
Pull request description:
Before it was 0 by default for main and 20000 for test and regtest.
Now it is 0 by default for all chains, thus there's no need to call Params().
Also now the default for main is properly documented.
Suggestion for release notes:
-fallbackfee was 0 (disabled) by default for the main chain, but 20000 by default for the test chains. Now it is 0 by default for all chains. Testnet and regtest users will have to add fallbackfee=20000 to their configuration if they weren't setting it and they want it to keep working like before.
Should I propose them to the wiki for the release notes or only after merge?
For more context, see https://github.com/bitcoin/bitcoin/pull/16402#issuecomment-515701042
ACKs for top commit:
MarcoFalke:
ACK ea4cc3a7b3
Tree-SHA512: fdfaba5d813da4221e405e0988bef44f3856d10f897a94f9614386d14b7716f4326ab8a6646e26d41ef3f4fa61b936191e216b1b605e9ab0520b0657fc162e6c
Before it was 0 by default for main and 20000 for test and regtest.
Now it is 0 by default for all chains, thus there's no need to call Params().
Also now the default for main is properly documented
faec689bed txmempool: Make entry time type-safe (std::chrono) (MarcoFalke)
faaa1f01da util: Add count_seconds time helper (MarcoFalke)
1111170f2f test: mempool entry time is persisted (MarcoFalke)
Pull request description:
This changes the type of the entry time of txs into the mempool from `int64_t` to `std::chrono::seconds`.
The benefits:
* Documents the type for developers
* Type violations result in compile errors
* After compilation, the two are equivalent (at no run time cost)
ACKs for top commit:
ajtowns:
utACK faec689bed
laanwj:
ACK faec689bed
Tree-SHA512: d958e058755d1a1d54cef536a8b30a11cc502b7df0d6ecf84a0ab1d38bc8105a67668a99cd5087a444f6de2421238111c5fca133cdf8e2e2273cb12cb6957845
c812aba394 test bumpfee fee_rate argument (ezegom)
9f25de3d9e rpc bumpfee check fee_rate argument (ezegom)
88e5f997df rpc bumpfee: add fee_rate argument (ezegom)
1a4c791cf4 rpc bumpfee: move feerate estimation logic into separate method (ezegom)
Pull request description:
Taking over for https://github.com/bitcoin/bitcoin/pull/16492 which seems to have gone inactive.
Only minor commit cleanups, rebase, and some help text fixes on top of previous PR. Renamed `feeRate` to `fee_rate` to reflect updated guidelines.
ACKs for top commit:
Sjors:
Code review ACK c812aba
laanwj:
ACK c812aba394
Tree-SHA512: 5f7f51bd780a573ccef1ccd72b0faf3e5d143f6551060a667560c5163f7d9480e17e73775d1d7bcac0463f3b6b4328f0cff7b27e39483bddc42a530f4583ce30
9c23ebd6b1 qa: Fix service flag comparison check in rpc_net test (Luke Dashjr)
Pull request description:
Rebase of #16936
ACKs for top commit:
darosior:
ACK 9c23ebd6b1
Tree-SHA512: 74f287740403da1040ab1e235ef6eba4e304f3ee5d57a3b25d1e2e1f2f982d256528d398a4d6cb24ba393798e680a8f46cd7dae54ed84ab2c747e96288f1f884
43e7d576f5 doc: Improve test READMEs (Fabian Jahr)
Pull request description:
General improvements on READMEs for unit tests and functional tests:
- Give unit test readme a headline
- Move general information on `src/test` folder to the top
- Add information on logging and debugging unit tests
- Improve debugging and logging information in functional testing
- Include all available log levels in functional tests
ACKs for top commit:
laanwj:
ACK 43e7d576f5
Tree-SHA512: 22b27644992ba5d99a885cd51b7a474806714396fcea1fd2d6285e41bdf3b28835ad8c81449099e3ee15a63d57b3ab9acb89c425d9855ed1d9b4af21db35ab03
1a02edb3f2 [RPC] Fix casing in getblockchaininfo to be inline with the rest of the response (Dan Gershony)
Pull request description:
The response in the RPC result `startTime` is camel cased while the rest of the response seems to be lower cased.
If this was intentional please ignore and close this PR.
Note: RPC field case changes might break existing callers
ACKs for top commit:
laanwj:
ACK 1a02edb3f2
Tree-SHA512: 6f0eaf2b4aaf73c9a9bf1fbd4af59af5f95fc012fa88f94e050e6ae273b3ad647f5729df53bfce91e1a925fe4fd7b14818908bb6131a81413a555137d1007d7c
The response in the RPC result `starttime` is camel cased while the rest of the response seems to be lower cased.
If this was intentional please ignore this PR.
Note: case might break existing callers
Reflect the change in the test data
Change to snake case
f4beb4996d test: Remove python dead code linter (Wladimir J. van der Laan)
Pull request description:
Primarily I'd like to remove this because it is very imprecise, due to Python's dynamic nature, giving it a large list of false positives that need to be listed as exceptions. See for example #16906.
It's also a frequent source of complaints. I'm doubtful of the usefulness of checking for dead code in a linter in the first place.
Having some dead code in the test framework for a while is not a
disaster.
ACKs for top commit:
sdaftuar:
utACK f4beb4996d
practicalswift:
ACK f4beb4996d -- diff looks correct
jamesob:
ACK f4beb4996d
Tree-SHA512: 329b1555210311d5d15799fd2cb794b3208b0ac4d8a2ffaf4dece1bcc3e0e8b1fe952d5e7a394f94a98919cab579fb579eae7db2a796cc9a1a42ef495dd17507
6659810e2f test: use named args for sendrawtransaction calls (Jon Atack)
5c1cd78b7e doc: improve rawtransaction code/test docs (Jon Atack)
acc14c5093 test: fix incorrect value in rpc_rawtransaction.py (Jon Atack)
Pull request description:
Follow-up to PR #16521.
- Fix incorrect value in rpc_rawtransaction test as per https://github.com/bitcoin/bitcoin/pull/16521/files#r325842308
- Improve the code docs
- Use named arguments as per https://github.com/bitcoin/bitcoin/pull/16521/files#r310715127
Happy to squash or keep only the first commit if the others are too fixup-y.
ACKs for top commit:
laanwj:
ACK 6659810e2f
Tree-SHA512: bf5258f23802ab3ba3defb8791097e08e63f3e2af21023f832cd270dc88d1fa04349e921d69f9f5fedac5dce5cd3c1cc46b48febbede4bc18dccb8be994565b2
Primarily I'd like to remove this because it is very imprecise, due to
Python's dynamic nature, giving it a large list of false positives that
need to be listed as exceptions. See for example #16906.
It's also a frequent source of complaints. I'm doubtful of the
usefulness of checking for dead code in a linter in the first place.
Having some dead code in the test framework for a while is not a
disaster.
fa69588537 test: Make PORT_MIN in test runner configurable (MarcoFalke)
Pull request description:
This is needed when some ports in the port range are used by other processes. Note that simply assigning the ports dynamically does not work:
* We spin up several nodes per test (each node gets its own port)
* We run several tests in parallel
So to avoid nodes from different tests colliding on ports, the port assignment must be deterministic (can not be dynamic).
Fixes: #10869
ACKs for top commit:
practicalswift:
ACK fa69588537 -- diff looks correct
promag:
ACK fa69588537.
Tree-SHA512: e79adb015e7de79064e2d14336c38bc9672bd779ad6c52917721897e73f617c39d32c068a369c26670002a6c4ab95a71ef3a6878ebdd9710e02f410e2f7bcd14
fa2e038691 test: Fix extra_args in wallet_import_rescan.py (MarcoFalke)
Pull request description:
Bug introduced by me (🤦♂️) in fa25668e1c
For reference:
```
>>> a = [[]]*2
>>> a[0] += ['ONE']
>>> a
[['ONE'], ['ONE']]
>>> a = [[] for _ in range(2)]
>>> a[0] += ['ONE']
>>> a
[['ONE'], []]
ACKs for top commit:
theStack:
utACK fa2e038
Tree-SHA512: 7d75a0d06233d013d62198ea95793612242254d5d90f393d01b2beef5abc78d6e85c796532311638f16cfed3b66a7ae41a108c0fe6f0f5d7f6616b042c670df7
c4b0c08f7c Update tx-size-small comment with relevant CVE disclosure (Gregory Sanders)
Pull request description:
Code first introduced under https://github.com/bitcoin/bitcoin/pull/11423 with essentially no description and no discussion.
ACKs for top commit:
MarcoFalke:
ACK c4b0c08f7c
fanquake:
ACK c4b0c08f7c
Tree-SHA512: 95d5c92998b8b1e944c477dbaee265b62612b6e815099ab31d9ff580b4dff777abaf7f326a284644709f918aa1510412d62310689b1250ef6e64de7b19ca9f71
fadfd844de test: Remove unused connect_nodes_bi (MarcoFalke)
fa3b9ee8b2 scripted-diff: test: Replace connect_nodes_bi with connect_nodes (MarcoFalke)
faaee1e39a test: Use connect_nodes when connecting nodes in the test_framework (MarcoFalke)
1111bb91f5 test: Reformat python imports to aid scripted diff (MarcoFalke)
Pull request description:
By default all test nodes are connected in a chain. However, instead of just a single connection between each pair of nodes, we end up with up to four connections for a "middle" node (two outbound, two inbound, from each side).
This is generally redundant (tx and block relay should succeed with just a single connection) and confusing. For example, test timeouts after a call to `sync_` may be racy and hard to reproduce. On top of that, the test `debug.log`s are hard to read because txs and block invs may be relayed on the same connection multiple times.
Fix this by inlining `connect_nodes_bi` in the two tests that need it, and then replace it with a single `connect_nodes` in all other tests.
Historic background:
`connect_nodes_bi` has been introduced as a (temporary?) workaround for bug #5113 and #5138, which has long been fixed in #5157 and #5662.
ACKs for top commit:
laanwj:
ACK fadfd844de
jonasschnelli:
utACK fadfd844de - more of less a cleanup PR.
promag:
Tested ACK fadfd844de, ran extended tests.
Tree-SHA512: 2d027a8fd150749c071b64438a0a78ec922178628a7dbb89fd1212b0fa34febd451798c940101155d3617c0426c2c4865174147709894f1f1bb6cfa336aa7e24
fac35b21e2 test: lint: Add DisabledOpcodeTemplates to whitelist (MarcoFalke)
Pull request description:
Fixes#16906
Top commit has no ACKs.
Tree-SHA512: 98b175bb062425fd3a8bd0d0258f4c0e0d5106980f1e037df7c2b2b2e5aa6031b11b582c026265d7b2de56049ccbadb0b7add9130d323f15886f681c6268ba0a
2dfd6834ef test: Add test for default maxfeerate in sendrawtransaction (Joonmo Yang)
261843e4be wallet/rpc: Use the default maxfeerate value as BTC/kB (Joonmo Yang)
Pull request description:
Fixes https://github.com/bitcoin/bitcoin/issues/16382
This patch tries to treat `maxfeerate` in sendrawtransaction/testmempoolaccept RPC as a rate(BTC/kB) instead of an absolute value(BTC).
The included test case checks if the new behavior works correctly, by using the transaction with an absolute fee of ~0.02BTC, where the fee rate is ~0.2BTC/kB.
This test should be failing if the default `maxfeerate` is 0.1BTC, but pass if the default value is 0.1BTC/kB
ACKs for top commit:
laanwj:
ACK 2dfd6834ef (ACKs by Sjors and MarcoFalke above for trivially different code)
Tree-SHA512: a1795bffe8a182acef8844797955db1f60bb0c0ded97148f3572dc265234d5219271a3a7aa0b6418a43f73b2b2720ef7412ba169c99bb1cdcac52051f537d6af
c0b5d97103 Test that joinpsbts randomly shuffles the inputs (Andrew Chow)
6f405a1d3b Shuffle inputs and outputs after joining psbts (Andrew Chow)
Pull request description:
`joinpsbts` currently just adds the inputs and outputs in the order of that the PSBTs were provided. This makes it extremely easy to identify which outputs belong to which inputs. This PR changes that so that all of the inputs and outputs are shuffled in the joined transaction.
ACKs for top commit:
instagibbs:
utACK c0b5d97103
jonatack:
ACK c0b5d97103 modulo suggestions for later.
Tree-SHA512: 14a0b7aae07d92e6d2c76a3a3b228b481e1964cb7d34f97515bdda18e2ea05a9f97c5a22affc143b86ae8b95c3cb239849fb54219d65512bc2112264dca915c8
0c62e3aa73 New regression testing for CVE-2018-17144, CVE-2012-2459, and CVE-2010-5137. (lucash-dev)
38bfca6bb2 Added comments referencing multiple CVEs in tests and production code. (lucash-dev)
Pull request description:
This functional test includes two scenarios that test for regressions of vulnerabilities, but they are only briefly described. There are freely available documents explaining in detail the issues, but without explicit mentions, the developer trying to maintain the code needs an additional step of digging in commit history and PR conversations to figure it out.
Added comments to explicitly mention CVE-2018-17144 and CVE-2012-2459, for more complete documentation.
This improves developer experience by making understanding the tests easier.
ACKs for top commit:
laanwj:
ACK 0c62e3aa73, checked the CVE numbers, thanks for adding documentation
Tree-SHA512: 3ee05351745193b8b959e4a25d50f25a693b2d24b0732ed53cf7d5882df40b5dd0f1877bd5c69cffb921d4a7acf9deb3cc1160b96dc730d9b5984151ad06b7c9
ae0add8dfe Add python bech32 impl round-trip test (Gregory Sanders)
Pull request description:
Currently there is a single use of `segwit_addr.encode`, and zero uses of `segwit_addr.decode` in the codebase.
This adds a simple round-trip test of the implementation to avoid future regressions.
Top commit has no ACKs.
Tree-SHA512: feb3303f240f5987993e092ec15b878c8db3957d338db6a08fbe947bbfea0c558c7ebc26f8052c38a69d85c354f24e71431e19e0a2991c3c64b604f6d50697ff
2222c96dee test: Add notes on how to generate data/wallets/high_minversion (MarcoFalke)
Pull request description:
I forgot to do this in #16796
ACKs for top commit:
ryanofsky:
ACK 2222c96dee
Tree-SHA512: 5f24ffa641b97eac4febad42ade7228b14fa72335c918a10880c5dec86a3ecc3075a31526f275188e07fea95b8e2c6320c64f716099f604b00e13d5366fcee37
fae961de6b test: Establish only one connection between nodes in rpc_invalidateblock (MarcoFalke)
Pull request description:
Headers and block sync should eventually converge to the same result, regardless of whether the peers treat each other as "inbound" or "outbound".
`connect_nodes_bi` has been introduced as a (temporary?) workaround for bug #5113 and #5138, which has long been fixed in #5157 and #5662.
Thus remove the `connect_nodes_bi` workaround from the rpc_invalidateblock test.
Conveniently, this also closes#16453. See https://github.com/bitcoin/bitcoin/issues/16444#issuecomment-514801708 for rationale
ACKs for top commit:
laanwj:
ACK fae961de6b
Tree-SHA512: b3614c66a205823df73f64d19cacfbec269beb5db52ff79004d746e17d7c0dfb43ab9785fdddc97e2a76fe76286c8c605b34df3dda4a2bf5be035f01169ae89a
- Test gettransaction response without verbose, with verbose=False, and with verbose=True.
- In each case, test presence of expected fields in the output, including absence of the "decoded" field when `verbose` is not passed or false.
- Test that the "details" field contains the expected receive vout in each case.
PR 16866 renamed the 'decode' argument in gettransaction to 'verbose' to make it more consistent with other RPC calls like getrawtransaction.
However, it seems it inadvertently overloaded the 'details' fields when 'verbose' is passed. The result is that the original 'details' fields are no longer returned, which seems to be a breaking API change.
This PR takes the simplest path to restoring the 'details' fields by renaming them from 'details' back to 'decoded', while leaving the 'verbose' argument for API consistency.
It also addresses [this comment](https://github.com/bitcoin/bitcoin/pull/16185#discussion_r320740413) to mention that the 'decoded' field is identical to decoderawtransaction.
Update the RPC help, functional test, and release note.
This makes the RPC method consistent with other RPC methods that have a
'verbose' option.
Change the name of the return object from 'decoded' to details.
Update help text.
333317ce6b test: Test that low difficulty chain fork is rejected (MarcoFalke)
fa31dc1bf4 test: Pass down correct chain name in tests (MarcoFalke)
Pull request description:
To prevent OOM, Bitcoin Core will reject chain forks at low difficulty by default. This is the only use-case of checkpoints, so add a test for it to make sure the feature works as expected. If it didn't work, checkpoints would have no use-case and we might as well remove them
ACKs for top commit:
Sjors:
Thanks for adding the node 1 example. Code review ACK 333317c
Tree-SHA512: 90dffa540d0904f3cffb61d2382b1a26f84fe9560b7013e4461546383add31a8757b350616a6d43217c59ef7b8b2a1b62bb3bab582c679cbb2c660a782ce7be1
1d524c62ea tests: rename 'test_getnetworkinginfo' in 'test_getnetworkinfo' (darosior)
07a8f65031 tests: add a test for the 'servicesnames' RPC field (darosior)
Pull request description:
As per https://github.com/bitcoin/bitcoin/pull/16787#issuecomment-529801457, fixes#16844.
This adds a test for both commands in the first commit and renames the test for `getnetworkinfo` in the second commit.
ACKs for top commit:
laanwj:
ACK 1d524c62ea
Tree-SHA512: 8267dce4d54356debab75014e6f9ba885b892da605ed32f26a5446c232992fcae761861bb678adbdb942815d4706f3768c70deee6afec68f219b23605475be01
ec4c79326b signrawtransaction*: improve error for partial signing (Anthony Towns)
3c481f8921 signrawtransactionwithkey: better error messages for bad redeemScript/witnessScript (Anthony Towns)
Pull request description:
Two fixes for `signrawtransactionwith{key,wallet}` (in addition to #16250): one that checks redeemScript/witnessScript matches scriptPubKey (and if both are provided that they match each other sanely), and the other changes the warning when some-but-not-all the signatures for a CHECKMULTISIG are provided to something that suggests more signatures may be all that's required.
Fixes: #13218Fixes: #14823
ACKs for top commit:
instagibbs:
utACK ec4c79326b
achow101:
Code Review ACK ec4c79326b
meshcollider:
utACK ec4c79326b
Tree-SHA512: 0c95c91d498e85b834662b9e5c83f336ed5fd306be7701ce1dbfa0836fbeb448a267a796585512f7496e820be668b07c2a0a2f45e52dc23f09ee7d9c87e42b35
This adds checks to ensure the redeemScript/witnessScript actually
correspond to the provided scriptPubKey, and, if both are provided,
that they are sensibly related to each other.
Thanks to github user passionofvc for raising this issue.
6d803494b5 Don't show addresses or P2PK in decoderawtransaction (nicolas.dorier)
Pull request description:
I spent significant amount of time explaining to people that satoshi did not had any "bitcoin address", because bitcoin address was not existing at the time.
Then I need to explain them that all blockchain explorer are wrong. Then I understood that the source of this widespread mistake come from Bitcoin Core itself.
For:
```
bitcoin-cli -regtest decoderawtransaction 01000000010000000000000000000000000000000000000000000000000000000000000000ffffffff4d04ffff001d0104455468652054696d65732030332f4a616e2f32303039204368616e63656c6c6f72206f6e206272696e6b206f66207365636f6e64206261696c6f757420666f722062616e6b73ffffffff0100f2052a01000000434104678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef38c4f35504e51ec112de5c384df7ba0b8d578a4c702b6bf11d5fac00000000
```
Before:
```json
{
"txid": "4a5e1e4baab89f3a32518a88c31bc87f618f76673e2cc77ab2127b7afdeda33b",
"hash": "4a5e1e4baab89f3a32518a88c31bc87f618f76673e2cc77ab2127b7afdeda33b",
"version": 1,
"size": 204,
"vsize": 204,
"weight": 816,
"locktime": 0,
"vin": [
{
"coinbase": "04ffff001d0104455468652054696d65732030332f4a616e2f32303039204368616e63656c6c6f72206f6e206272696e6b206f66207365636f6e64206261696c6f757420666f722062616e6b73",
"sequence": 4294967295
}
],
"vout": [
{
"value": 50.00000000,
"n": 0,
"scriptPubKey": {
"asm": "04678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef38c4f35504e51ec112de5c384df7ba0b8d578a4c702b6bf11d5f OP_CHECKSIG",
"hex": "4104678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef38c4f35504e51ec112de5c384df7ba0b8d578a4c702b6bf11d5fac",
"reqSigs": 1,
"type": "pubkey",
"addresses": [
"mpXwg4jMtRhuSpVq4xS3HFHmCmWp9NyGKt"
]
}
}
]
}
```
After
```json
{
"txid": "4a5e1e4baab89f3a32518a88c31bc87f618f76673e2cc77ab2127b7afdeda33b",
"hash": "4a5e1e4baab89f3a32518a88c31bc87f618f76673e2cc77ab2127b7afdeda33b",
"version": 1,
"size": 204,
"vsize": 204,
"weight": 816,
"locktime": 0,
"vin": [
{
"coinbase": "04ffff001d0104455468652054696d65732030332f4a616e2f32303039204368616e63656c6c6f72206f6e206272696e6b206f66207365636f6e64206261696c6f757420666f722062616e6b73",
"sequence": 4294967295
}
],
"vout": [
{
"value": 50.00000000,
"n": 0,
"scriptPubKey": {
"asm": "04678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef38c4f35504e51ec112de5c384df7ba0b8d578a4c702b6bf11d5f OP_CHECKSIG",
"hex": "4104678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef38c4f35504e51ec112de5c384df7ba0b8d578a4c702b6bf11d5fac",
"reqSigs": 1,
"type": "pubkey",
"addresses": [
]
}
}
]
}
```
This mistake is having widespread impact, as developer thinks P2PK are addresses, they start running into issues when somebody send a P2PK payment to them and then they don't understand why they can't sign it like a P2PKH.
ACKs for top commit:
Sjors:
Code review ACK 6d80349.
MarcoFalke:
ACK 6d803494b5
meshcollider:
utACK 6d803494b5
kristapsk:
ACK 6d803494b5 (applied changes except test, ran tests, then applied changes to test also)
Tree-SHA512: 6e4990164a6b8df6675f09b2b189b7197fad43f1918fc1a4530ebd98ce71c3c94d9ec54e1b4624210fd7c5200d4f04825ca37f4e42f5fe9b8a9c0f38c50591ef
fa734603b7 wallet: Fix segmentation fault in CreateWalletFromFile (MarcoFalke)
fab3c34412 test: Print both messages on failure in assert_raises_message (MarcoFalke)
faa13539d5 wallet: Fix documentation around WalletParameterInteraction (MarcoFalke)
Pull request description:
Comes with a test to aid review. The test should fail without the fix to bitcoind
The following `CreateWalletFromFile` issues are fixed:
* `walletFile` refers to freed memory and will thus corrupt the debug.log and/or crash the node if read
* `WalletParameterInteraction` was moved to `CreateWalletFromFile` and `WalletInit::ParameterInteraction` without updating the documentation
ACKs for top commit:
promag:
ACK fa734603b7.
darosior:
ACK fa734603b7
meshcollider:
LGTM, code-read ACK fa734603b7
Tree-SHA512: 2aceb63a3f25b90a840cfa08d37f5874aad4eb3df8c2ebf94e2ed18b55809b185e6920bdb345b988bff1fcea5e68a214fe06c361f7da2c01a3cc29e0cc421cb4
bdd6a4fd5d qa: Check scantxoutset result against gettxoutsetinfo (João Barbosa)
fc0c410d6e rpc: Improve scantxoutset response and help message (João Barbosa)
Pull request description:
The new response keys `height` and `bestblock` allow the client to know at what point the scan took place.
The help message now has all the response keys (`result` and `txouts` were missing) and it's improved a bit. Note that `searched_items` key is renamed to `txouts`, considering `scantxoutset` is marked experimental.
ACKs for top commit:
laanwj:
ACK bdd6a4fd5d
Tree-SHA512: 6bb7c3464b19857b756b8bc491ab7c58b0d948aad8c005b26ed27c55a1278f5639217e11a315bb505b4f44ebe86f413068c1e539c8a5f7a4007735586cc6443c
0ba08020c9 Disconnect peers violating blocks-only mode (Suhas Daftuar)
937eba91e1 doc: improve comments relating to block-relay-only peers (Suhas Daftuar)
430f489027 Don't relay addr messages to block-relay-only peers (Suhas Daftuar)
3a5e885306 Add 2 outbound block-relay-only connections (Suhas Daftuar)
b83f51a4bb Add comment explaining intended use of m_tx_relay (Suhas Daftuar)
e75c39cd42 Check that tx_relay is initialized before access (Suhas Daftuar)
c4aa2ba822 [refactor] Change tx_relay structure to be unique_ptr (Suhas Daftuar)
4de0dbac9b [refactor] Move tx relay state to separate structure (Suhas Daftuar)
26a93bce29 Remove unused variable (Suhas Daftuar)
Pull request description:
Transaction relay is optimized for a combination of redundancy/robustness as well as bandwidth minimization -- as a result transaction relay leaks information that adversaries can use to infer the network topology.
Network topology is better kept private for (at least) two reasons:
(a) Knowledge of the network graph can make it easier to find the source IP of a given transaction.
(b) Knowledge of the network graph could be used to split a target node or nodes from the honest network (eg by knowing which peers to attack in order to achieve a network split).
We can eliminate the risks of (b) by separating block relay from transaction relay; inferring network connectivity from the relay of blocks/block headers is much more expensive for an adversary.
After this commit, bitcoind will make 2 additional outbound connections that are only used for block relay. (In the future, we might consider rotating our transaction-relay peers to help limit the effects of (a).)
ACKs for top commit:
sipa:
ACK 0ba08020c9
ajtowns:
ACK 0ba08020c9 -- code review, ran tests. ran it on mainnet for a couple of days with MAX_BLOCKS_ONLY_CONNECTIONS upped from 2 to 16 and didn't observe any unexpected behaviour: it disconnected a couple of peers that tried sending inv's, and it successfully did compact block relay with some block relay peers.
TheBlueMatt:
re-utACK 0ba08020c9. Pointed out that stats.fRelayTxes was sometimes uninitialized for blocksonly peers (though its not a big deal and only effects RPC), which has since been fixed here. Otherwise changes are pretty trivial so looks good.
jnewbery:
utACK 0ba08020c9
jamesob:
ACK 0ba08020c9
Tree-SHA512: 4c3629434472c7dd4125253417b1be41967a508c3cfec8af5a34cad685464fbebbb6558f0f8f5c0d4463e3ffa4fa3aabd58247692cb9ab8395f4993078b9bcdf
5ce822efbe Conservatively accept RBF bumps bumping one tx at the package limits (Matt Corallo)
Pull request description:
Based on #15681, this adds support for some simple cases of RBF inside of large packages. Issue pointed out by sdaftuar in #15681, and this fix (or a broader one) is required ot make #15681 fully useful.
Accept RBF bumps of single transactions (ie which evict exactly one
transaction) even when that transaction is a member of a package
which is currently at the package limit iff the new transaction
does not add any additional mempool dependencies from the original.
This could be made a bit looser in the future and still be safe,
but for now this fixes the case that a transaction which was
accepted by the carve-out rule will not be directly RBF'able
ACKs for top commit:
instagibbs:
re-ACK 5ce822efbe
ajtowns:
ACK 5ce822efbe ; GetSizeWithDescendants is only change and makes sense
sipa:
Code review ACK 5ce822efbe. I haven't thought hard about the effect on potential DoS issues this policy change may have.
Tree-SHA512: 1cee3bc57393940a30206679eb60c3ec8cb4f4825d27d40d1f062c86bd22542dd5944fa5567601c74c8d9fd425333ed3e686195170925cfc68777e861844bd55
442a87cc0a Add a test wallet_reorgsrestore (Antoine Riard)
40ede992d9 Modify wallet tx status if has been reorged out (Antoine Riard)
7e89994133 Remove SyncTransaction for conflicted txn in CWallet::BlockConnected (Antoine Riard)
a31be09bfd Encapsulate tx status in a Confirmation struct (Antoine Riard)
Pull request description:
While working on #15931, I've tried to rationalize tx state management to ease integration of block height tracking per-wallet tx. We currently rely on a combination of `hashBlock` and `nIndex` with magic value to determine tx confirmation, conflicted or abandoned state. It's hard to reason and error-prone. To solve that, we encapsulate these fields in a `TxConfirmation` struct and introduce a `TxState` member that we update accordingly at block connection/disconnection.
Following jnewbery [recommendation](https://github.com/bitcoin/bitcoin/pull/15931#discussion_r312576506), I've taken these changes in its own commit, and open a PR to get them first. It would ease review of aforementioned PR, but above all should ease fixing of long-term issues like :
* https://github.com/bitcoin/bitcoin/issues/7315 (but maybe we should abandon abandontransaction or relieve it to only free outpoints not track the transaction as abandoned in itself, need its own discussion)
* https://github.com/bitcoin/bitcoin/issues/8692 where we should cancel conflicted state of transactions chain smoothly
* `MarkConflicted` in `LoadToWallet` is likely useless if we track conflicts rights at block connection
Main changes of this PR to get right are tx update in `AddToWallet` and serialization/deserialization logic.
ACKs for top commit:
meshcollider:
Light re-Code Review ACK 442a87cc0a
ryanofsky:
utACK 442a87cc0a. Changes since last review are switching from `hasChain` to `LockChain` and removing chain lock in `WalletBatch::LoadWallet` that's redundant with the new lock still added in `CWallet::LoadWallet`, and fixing python test race condition.
Tree-SHA512: 029209e006de0240436817204e69e548c5665e2b0721b214510e7aba7eba130a5eab441d3a1ad95bd6426114dd27390492c77bf4560a9610009b32cd0a1f72f7
fae91a09c4 test: Remove incorrect and unused try-block in assert_debug_log (MarcoFalke)
Pull request description:
This try block has accidentally been added by me in fa3e9f7627.
It was unused all the time, but commit 6011c9d72d added a `return` in the finally block, muting all exceptions.
This can be tested by adding an `assert False` after any `with ...assert_debug_log...:` line.
ACKs for top commit:
laanwj:
ACK fae91a09c4
ryanofsky:
utACK fae91a09c4. I didn't know returning inside a `finally` block would cancel pending exceptions or return values, but I guess this makes sense and is a good thing to be aware of.
Tree-SHA512: 47ed0165062060e9af055a3e92f1a529cd41d00476bfad64e3cd141ae084d22f926a343bb1257717e164e15459a59ab66aed198c95d18bf780d8cb0b76aa3298
3d0a82cff8 devtools: Accomodate block-style copyright blocks (Ben Woosley)
0ef0e51fe4 lint: Bump flake8 to 3.7.8 (Ben Woosley)
838920704a lint: Disable flake8 W504 warning (Ben Woosley)
b21680baf5 test/contrib: Fix invalid escapes in regex strings (Ben Woosley)
Pull request description:
This is a second go at #15221, fixing new lints in:
W504 line break after binary operator
W605 invalid escape sequence
F841 local variable 'e' is assigned to but never used
This time around:
* One commit per rule, for easier review
* I went with the PEP-8 style of breaking before binary operators
* I looked into the raw regex newline issue, and found that raw strings with newlines embedded do work appropriately. E.g. run `re.match(r" \n ", " \n ")` to check this for yourself. `re.MULTILINE` exists to modify `^` and `$` in multiline scenarios, but all of these searches are per-line.
ACKs for top commit:
practicalswift:
ACK 3d0a82cff8 -- diff looks correct
Tree-SHA512: bea0c144cadd72e4adf2e9a4b4ee0535dd91a8e694206924cf8a389dc9253f364a717edfe9abda88108fbb67fda19b9e823f46822d7303c0aaa72e48909a6105
490da639cb Make lint-includes.sh work from any directory (Kristaps Kaupe)
Pull request description:
Before this change it works from root folder of bitcoin git repo, but if you do `cd test/lint; ./test-includes.sh`, you will have a lot of false positive messages like this:
```
Good job! The circular dependency "chainparamsbase -> util/system -> chainparamsbase" is no longer present.
Please remove it from EXPECTED_CIRCULAR_DEPENDENCIES in ./lint-circular-dependencies.sh
to make sure this circular dependency is not accidentally reintroduced.
Good job! The circular dependency "index/txindex -> validation -> index/txindex" is no longer present.
Please remove it from EXPECTED_CIRCULAR_DEPENDENCIES in ./lint-circular-dependencies.sh
to make sure this circular dependency is not accidentally reintroduced.
```
Top commit has no ACKs.
Tree-SHA512: 07fa69cb2883181dcee922191acac4b242722eeb2916cdffdc7163421302b22f3c9525aaf4c754a9dba1c307032c05285e38191d5c6aabc894321f8a27bbceaa
Accept RBF bumps of single transactions (ie which conflict with one
transaction) even when that transaction is a member of a package
which is currently at the package limit iff the new transaction
does not add any additional mempool dependencies from the original.
This could be made a bit looser in the future and still be safe,
but for now this fixes the case that a transaction which was
accepted by the carve-out rule will not be directly RBF'able.
If we set fRelay=false in our VERSION message, and a peer sends an INV or TX
message anyway, disconnect. Since we use fRelay=false to minimize bandwidth,
we should not tolerate remaining connected to a peer violating the protocol.
Flagged by flake8 v3.6.0, as W605, plus a few others identified
incidentally, e.g. 59ffecf66cf4d08c4b431e457b083878d66a3fd6.
Note that r"\n" matches to "\n" under re.match/search.
9965940e35 doc: Add release note for the new gettransaction argument (darosior)
b8b3f0435a tests: Add a new functional test for gettransaction (darosior)
7f3bb247a8 gettransaction: add an argument to decode the transaction (darosior)
Pull request description:
This PR adds a new parameter to the `gettransaction` call : `decode`. If set to `true`, it will add a new `decoded` field to the response. This mimics the behavior of `getrawtransaction`'s `verbose` argument to avoid using 2 calls if we want to decode a wallet transaction (`gettransaction` then `decoderawtransaction`).
Fix#16181 .
ACKs for top commit:
meshcollider:
re-utACK 9965940e35
Tree-SHA512: bcb6b4bd252b3488d6afc77659c499c2ad99fd58661eb24b6a2e17014c74f22e47fde70e00fedb4f4754915786622ad02483b2cf2c4dea0ab0eb4ac8276dbeee
d48c1e837a Add window final block height to getchaintxstats (Jonathan "Duke" Leto)
Pull request description:
This patch is motivated by the desire to make the output of `getchaintxstats` more useful and optimized for applications to consume and render the data.
Firstly, this data is already available to the RPC, no additional work is done. Currently additional RPC calls will be needed to look up the height of the final block in the window or the block height that began the window.
By adding the block height of the final block in the window, the JSON is "self-contained" and applications can calculate the exact block height range of the window with no additional RPC requests.
For example, a web application which wants to render historical information for `getchaintxstats` RPC on various window sizes might call the RPC with various window lengths, once per day, and store the JSON results somewhere. Because the final block height of each dataset is included, it's no extra work to determine the exact block window range of each JSON response.
ACKs for top commit:
promag:
ACK d48c1e837a.
Tree-SHA512: fd4952c125f81a4ad18f7c78498c6b3e265b93cb574832166ac25596321ce84957f971f3f78f37d7e42638dc65f2a5d4d760f289873c9c2f2a82eb00a0f87c3f
e4f4ea47eb lint: Catch use of [] or {} as default parameter values in Python functions (practicalswift)
25dd867150 Avoid using mutable default parameter values (practicalswift)
Pull request description:
Avoid common Python default parameter gotcha when mutable `dict`/`list`:s are used as default parameter values.
Examples of this gotcha caught during review:
* https://github.com/bitcoin/bitcoin/pull/16673#discussion_r317415261
* https://github.com/bitcoin/bitcoin/pull/14565#discussion_r241942304
Perhaps surprisingly this is how mutable list and dictionary default parameter values behave in Python:
```
>>> def f(i, j=[], k={}):
... j.append(i)
... k[i] = True
... return j, k
...
>>> f(1)
([1], {1: True})
>>> f(1)
([1, 1], {1: True})
>>> f(2)
([1, 1, 2], {1: True, 2: True})
```
In contrast to:
```
>>> def f(i, j=None, k=None):
... if j is None:
... j = []
... if k is None:
... k = {}
... j.append(i)
... k[i] = True
... return j, k
...
>>> f(1)
([1], {1: True})
>>> f(1)
([1], {1: True})
>>> f(2)
([2], {2: True})
```
The latter is typically the intended behaviour.
This PR fixes two instances of this and adds a check guarding against this gotcha going forward :-)
ACKs for top commit:
Sjors:
Oh Python... ACK e4f4ea47eb. Testing tip: swap the two commits.
Tree-SHA512: 56e14d24fc866211a20185c9fdb274ed046c3aed2dc0e07699e58b6f9fa3b79f6d0c880fb02d72b7fe5cc5eb7c0ff6da0ead33123344e1a872209370c2e49e3f
403e372407 qa: Relax so that the subscriber is ready before publishing zmq messages (João Barbosa)
Pull request description:
Prevents the syndrome "slow joiner" - see http://zguide.zeromq.org/py:all#sockets-and-patterns - by relaxing before publishing messages.
ACKs for top commit:
MarcoFalke:
unsigned ACK 403e372407
Tree-SHA512: 0e856accbc450a9b09160bdce5112b2103dc9436cc317d31fb1c9634ebd76823a300a2e727818057fb4d0a615271772ff23e80553a13e9aa1935500de5eeec5f
6011c9d72d QA: fix rpc_setban.py race (Jonas Schnelli)
Pull request description:
The new `rpc_setban.py` test failes regularly on CIs due to a race between injecting the ban and testing the log "on the other side".
The problem is, that the test immediately after the `addnode` command on node0 checks for the `dropped (banned)` entry on node1 (without giving some time).
Adding a 2 seconds sleep seems to solve the race (I guess there is no better event-driven delay).
Example of a failed test: https://bitcoinbuilds.org/index.php?ansilog=bf743910-103f-4b54-9a97-960c471061bd.log#l2906
Top commit has no ACKs.
Tree-SHA512: 680f8ea3e5ddb07e93f824f1aeff4a459e25e6c14715a39fc7670e50506d7cf25925348672c5c2d8ba3e1243ccf5effbc2456bcd094fb96868349f8d26e008f1
17d178fb94 doc: add release-notes for getmempoolentry weight field addition (fanquake)
9c9cc2bd20 qa: Add RPC tests for weight in mempool entry (Daniel Edgecumbe)
54aaa7883c RPC: add weight to mempool entry output (Daniel Edgecumbe)
Pull request description:
Rebase of #14649 (which itself was a rebase of #11256).
Squash the two test related commits, and swapped out `size` usage for `vsize`.
Added a commit with release notes.
ACKs for top commit:
emilengler:
Concept ACK 17d178f
instagibbs:
utACK 17d178fb94
meshcollider:
utACK 17d178fb94
Tree-SHA512: 1d354c9837e0ad0afa40325de9329b9e62688d5eab4d9e1cf9b46d8ae29d08f42d903ab37a41751c2ea8f9034231b21095881b1f5d911cb542b8b06bc85dc7cd
ffea41f530 Enable all tests in feature_config_args.py (Hennadii Stepanov)
66f5c17f8a Use CheckDataDirOption() for code uniformity (Hennadii Stepanov)
7e33a18a34 Fix datadir handling in bitcoin-cli (Hennadii Stepanov)
b28dada374 Fix datadir handling in bitcoin-qt (Hennadii Stepanov)
50824093bb Fix datadir handling in bitcoind (Hennadii Stepanov)
740d41ce9f Add CheckDataDirOption() function (Hennadii Stepanov)
c1f325126c Return absolute path early in AbsPathForConfigVal (Hennadii Stepanov)
Pull request description:
Fix#15240, see: https://github.com/bitcoin/bitcoin/issues/15240#issuecomment-487353760Fix#15745
Fix broken `feature_config_args.py` tests (disabled by MarcoFalke@fabe28a0cdcfa13e0e595a0905e3642a960d3077). All test are enabled now.
This PR is alternative to #13621.
User's `$HOME` directory is not touched unnecessarily now.
~To make reviewing easier only `bitcoind` code is modified (neither `bitcoin-cli` nor `bitcoin-qt`).~
Refs:
- https://github.com/bitcoin/bitcoin/issues/15745#issuecomment-479852569 by **laanwj**
- #16220
Top commit has no ACKs.
Tree-SHA512: 4a4cda10e0b67c8f374da0c9567003d2b566d948e7f8550fe246868b5794c15010e88ea206009480b9cd2f737f310a15e984f920730448f99a895893bed351df
b168dd30cf Bugfix: QA: Run tests with UPnP disabled (Luke Dashjr)
Pull request description:
This replaces #16560 by adding `upnp=0` to `bitcoin.conf` rather than passing it to nodes.
> Needed for builds configured with --enable-upnp-default
You can test this change using:
```bash
./configure --enable-upnp-default && make -j6 && test/functional/test_runner.py feature_config_args.py
```
on master the test will fail without this change.
ACKs for top commit:
practicalswift:
ACK b168dd30cf -- diff looks correct
Tree-SHA512: e639dd480dda2cffa19a679018c4bd7e4bd4d0f5e3001d6b407b833e3c166bde98b201063e267b8e45f8a20b0d53ec8bc028bec806b2357f9a7ba314cc4e2d07
3b05f0f70f Reformat p2p_permissions.py (nicolas.dorier)
ce7eac3cb0 [Fix] The default whitelistrelay should be true (nicolas.dorier)
Pull request description:
I thought `whitelistrelay` default was `false` when it is `true`.
The root of the issue come from the fact that all references to `DEFAULT_` are not in the scope of this file, so hard coding of default values are used everywhere in `net.cpp`. I think that in a separate PR we should fix that more fundamentally everywhere.
ACKs for top commit:
promag:
ACK 3b05f0f70f.
Sjors:
re-ACK 3b05f0f70f
Tree-SHA512: f4a75f986fa2adf1a5f1c91605e0d261f7ac5ac8535fb05437d83b8392dbcf5cc1a47d755adcf8ad8dc67a88de28060187200fd3ce06545261a5c7ec0fea831a
