76c092ff80 wallet: warn against accidental unsafe older() import (Sjors Provoost)
592157b759 test: move SEQUENCE_LOCKTIME flags to script (Sjors Provoost)
Pull request description:
[BIP 379](https://github.com/bitcoin/bips/blob/master/bip-0379.md) ([Miniscript](https://bitcoin.sipa.be/miniscript/)) allows relative height and time locks that have no consensus meaning in [BIP 68](https://github.com/bitcoin/bips/blob/master/bip-0068.mediawiki) (relative timelocks) / [BIP 112](https://github.com/bitcoin/bips/blob/master/bip-0112.mediawiki) (`CHECKSEQUENCEVERIFY`). This is (ab)used by some protocols, e.g. [by Lightning to encode extra data](https://delvingbitcoin.org/t/exploring-extended-relative-timelocks/1818/23), but is unsafe when used unintentionally: `older(65536)` is equivalent to `older(1)`.
This PR emits a warning when `importdescriptors` contains such a descriptor.
The first commit makes `SEQUENCE_LOCKTIME` flags reusable by other tests.
The main commit adds the `ForEachNode` helper to `miniscript.h` which is then used in the `MiniscriptDescriptor` constructor to check for `Fragment::OLDER` with unsafe values. These are stored in `m_warnings`, which the RPC code then collects via `Warnings()`.
It adds both a unit and functional test.
---
A previous version of this PR prevented the import, unless the user opted in with an `unsafe` flag. It also used string parsing in the RPC code.
---
Based on:
- [x] https://github.com/bitcoin/bitcoin/pull/33914
ACKs for top commit:
pythcoiner:
reACK 76c092ff80
achow101:
ACK 76c092ff80
rkrux:
lgtm re-ACK 76c092ff80
brunoerg:
reACK 76c092ff80
Tree-SHA512: 8e944e499bd4a43cc27eeb889f262b499b9b07aa07610f4a415ccb4e34a9110f9946646f446a54ac5bf17494d8d96a89e4a1fa278385db9b950468f27283e17a
Note that we unfortunately can't use a scripted diff here, as the
`sha256` symbol is also used for other instances (e.g. as function
in hashlib, or in the `UTXO` class in p2p_segwit.py).
Since the previous commit, CTransaction object calls to the
methods `.rehash()` and `.calc_sha256()` are effectively no-ops
if the returned value is not used, so we can just remove them.
Removes all legacy wallet specific functional tests.
Also removes the --descriptor and --legacy-wallet options as these are
no longer necessary with the legacy wallet removed.
In order to ensure that the change of nVersion to a uint32_t in the
previous commit has no effect, rename nVersion to version in this commit
so that reviewers can easily spot if a spot was missed or if there is a
check somewhere whose semantics have changed.
Review note: The changes are complete, because self.options.descriptors
is set to None in parse_args (test_framework.py).
A value of None implies -disablewallet, see the previous commit.
So if a call to add_wallet_options is missing, it will lead to a test
failure when the wallet is compiled in.
Passing a list of transactions `txlist` to `create_block` appends
them to the block, hence we don't need to do that manually anymore.
The merkle root calculation can also be removed, since this is done
in the end of the helper.
The previous diff touched most files in ./test/, so bump the headers to
avoid having to touch them again for a bump later.
-BEGIN VERIFY SCRIPT-
./contrib/devtools/copyright_header.py update ./test/
-END VERIFY SCRIPT-
satoshi_round will round down. To make the code easier to parse use
Decimal() where possible, which does not round. Or use int(), which
explicitly rounds down.
This avoids having to remember to set it whenever mocktime is used with
peer connections. Also, it might help avoiding disconnects when
attaching a debugger to a running test.
d438d609cd QA: Use GBT to get block versions correct (Luke Dashjr)
1df2cd1c8f QA: blocktools: Accept block template to create_block (Luke Dashjr)
Pull request description:
The goal here is to decouple unrelated tests from the details of block versions.
Currently, these tests are forcing specific versions of blocks for no real reason.
ACKs for top commit:
fjahr:
re-ACK d438d609cd
benthecarman:
ACK d438d60
Tree-SHA512: 523b1cd4dac8d65c88432e126ce7f60df96ca4b94f7ecc8e83ba4ffbade23e2afe7055fdf586ce3c195a533f2004e63fff83add4267b39473a581c9f1c6d5340
substitutes "for x in range(N):" by "for _ in range(N):"
indicates to the reader that a block is just repeated N times, and
that the loop counter is not used in the body
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
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
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.
Unnamed arguments are confusing as to what they mean without looking up
the function signature.
Since segwit is active by default in regtest, and all blocks are
serialized with witness (#15664, c459c5f), remove the argument
`with_witness=True` from all calls to `CBlock::serialize` and
`BlockTransactions::serialize`.
This diff has been created with a script, but is better reviewed without
a scripted diff.
sed -i --regexp-extended -e 's/block(_?[2a-z]*)\.serialize\([a-z_]*=?True/block\1.serialize(/g' $(git grep -l serialize ./test)
