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
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
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
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
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
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
