607076d01b test: remove confusing `MAX_BLOCK_BASE_SIZE` (Sebastian Falbesoner)
4af97c74ed test: introduce `get_weight()` helper for CBlock (Sebastian Falbesoner)
a084ebe133 test: introduce `get_weight()` helper for CTransaction (Sebastian Falbesoner)
Pull request description:
This is a very late follow-up PR to #10618, which removed the constant `MAX_BLOCK_BASE_SIZE` from the core implementation about four years ago (see also #10608 in why it was considered confusing and superfluous).
Since there is also no point in still keeping it in the functional test framework, the PR switches to weight-based accounting on the relevant test code parts and use `MAX_BLOCK_WEIGHT` instead for the block limit checks. To prepare that, the first two commits introduce `get_weight()` helpers for the classes CTransaction and CBlock, respectively.
ACKs for top commit:
MarcoFalke:
review ACK 607076d01b🚴
Tree-SHA512: d59aa0b6b3dfd0a849b8063e66de275d252f705f99e25cd3bf6daec028b47d946777ee5b42a060f5283cb18e917ac073119c2c0e11bbc21211f69ef0a6ed335a
905d672b74 test: use script_util helpers for creating P2W{PKH,SH} scripts (Sebastian Falbesoner)
285a65ccfd test: use script_util helpers for creating P2SH scripts (Sebastian Falbesoner)
b57b633b94 test: use script_util helpers for creating P2PKH scripts (Sebastian Falbesoner)
61b6a017a9 test: wallet util: fix multisig P2SH-P2WSH script creation (Sebastian Falbesoner)
Pull request description:
PR #18788 (commit 08067aebfd) introduced functions to generate output scripts for various types. This PR replaces all manual CScript creations in the P2PKH, P2SH, P2WPKH, P2WSH formats with those helpers in order to increase readability and maintainability over the functional test codebase. The first commit fixes a bug in the wallet_util helper module w.r.t. to P2SH-P2WSH script creation (the result is not used in any test so far, hence it can still be seen as refactoring).
The following table shows a summary of the output script patterns tackled in this PR:
| Type | master branch | PR branch |
| ---------- | ------------- | ------------- |
| P2PKH | `CScript([OP_DUP, OP_HASH160, hash160(key), OP_EQUALVERIFY, OP_CHECKSIG])` | `key_to_p2pkh_script(key)` |
| | `CScript([OP_DUP, OP_HASH160, keyhash, OP_EQUALVERIFY, OP_CHECKSIG])` | `keyhash_to_p2pkh_script(keyhash)` |
| P2SH | `CScript([OP_HASH160, hash160(script), OP_EQUAL])` | `script_to_p2sh_script(script)` |
| P2WPKH | `CScript([OP_0, hash160(key)])` | `key_to_p2wpkh_script(key)` |
| P2WSH | `CScript([OP_0, sha256(script)])` | `script_to_p2wsh_script(script)` |
Note that the `key_to_...` helpers can't be used if an invalid key size (not 33 or 65 bytes) is passed, which is the case in some rare instances where the scripts still have to be created manually.
Possible follow-up ideas:
* further simplify by identifying P2SH-wrapped scripts and using `key_to_p2sh_p2wpkh_script()` and `script_to_p2sh_p2wsh_script()` helpers
* introduce and use `key_to_p2pk_script()` helper for P2PK scripts
ACKs for top commit:
rajarshimaitra:
tACK 905d672b74
LarryRuane:
tACK 905d672b74
0xB10C:
ACK 905d672b74
MarcoFalke:
review ACK 905d672b74 🕹
Tree-SHA512: 7ccfe69699bc81168ac122b03536720013355c1b2fbb088355b616015318644c4d1cd27e20c4f56c89ad083ae609add4bc838cf6316794d0edb0ce9cf7fa0fd8
The constant `MAX_BLOCK_BASE_SIZE` has been removed from the
core implementation years ago due to being confusing and
superfluous, as it is implied by the block weight limit (see
PRs #10618 and #10608). Since there is also no point in
still keeping it in the functional test framework, we switch
to weight-based accounting on the relevant test code parts
and use `MAX_BLOCK_WEIGHT` instead for the block limit
checks.
Only allow "packages" with no conflicts, sorted in order of dependency,
and no more than 25 for now. Note that these groups of transactions
don't necessarily need to adhere to some strict definition of a package
or have any dependency relationships. Clients are free to pass in a
batch of 25 unrelated transactions if they want to.
+ fee, fee_expected, output_amount
+ Using value of coin['amount'] as decimal and removed 'int'
+ Removed unnecessary parentheses
+ Remove str() and use quotes
1be0b1fb2a test: add functional test for non-standard bare multisig txs (Sebastian Falbesoner)
Pull request description:
Approaches another missing functional test of issue #17394 (counterpart to unit test in PR #17502): A transaction is rejected by the mempool with reason `"bare-multisig"` if any of the outputs' scriptPubKey has bare multisig format (`M <PubKey1> <PubKey2> ... <PubKeyN> N OP_CHECKSIG`) and bitcoind is started with the argument `-permitbaremultisig=0`.
ACKs for top commit:
instagibbs:
utACK 1be0b1fb2a
kristapsk:
ACK 1be0b1fb2a
Tree-SHA512: 2cade68c4454029b62278b38d0f137c2605a0e4450c435cdda2833667234edd4406f017ed12fa8df9730618654acbaeb68b16dcabb9f5aa84bad9f1c76c6d476
A transaction is rejected by the mempool with reason "bare-multisig" if any of
the outputs' scriptPubKey has bare multisig format (M <PubKey1> <PubKey2> ...
<PubKeyN> N OP_CHECKSIG) and bitcoind is started with "-permitbaremultisig=0".
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.
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
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
This commit adds comments referencing multiple CVEs both in production and test code.
CVEs covered in this commit:
CVE-2010-5137
CVE-2010-5139
CVE-2010-5141
CVE-2012-1909
CVE-2012-2459
CVE-2012-3789
CVE-2018-17144
29aeed1734 Bugfix: test/functional/mempool_accept: Ensure oversize transaction is actually oversize (Luke Dashjr)
Pull request description:
Simply integer dividing results in an acceptable size if the limit isn't an exact multiple of the input size.
Use math.ceil to ensure the transaction is always oversize.
(This issue can be triggered by changing the address style used.)
Tree-SHA512: e45062b0e8a3e9cb08e9dac5275b68d86e4377b460f1b3b995944090a055b0542a6986826312ec0e223369838094e42e20d8614b5c2bab9975b9a6f749295b21
Simply integer dividing results in an acceptable size if the limit isn't an exact multiple of the input size.
Use math.ceil to ensure the transaction is always oversize.
MarcoFalke