1ef28b4f7c Make AnalyzePSBT next role calculation simple, correct (Gregory Sanders)
Pull request description:
Sniped test and alternative to https://github.com/bitcoin/bitcoin/pull/18220
Sjors documenting the issue:
```
A PSBT signed by ColdCard was analyzed as follows (see #17509 (comment))
{
"inputs": [
{
"has_utxo": true,
"is_final": false,
"next": "finalizer"
}
],
"estimated_vsize": 141,
"estimated_feerate": 1e-05,
"fee": 1.41e-06,
"next": "signer"
}
I changed AnalyzePSBT so that it returns "next": "finalizer" instead.
```
It makes it much clearer that the role has been decided before hitting the `calc_fee` block, and groups all state-deciding in one spot instead of 2.
Note that this assumes that PSBT roles are a complete ordering, which for now and in the future seems to be a correct assumption.
ACKs for top commit:
Sjors:
ACK 1ef28b4f7c, much nicer. Don't forget to document the bug fix.
achow101:
ACK 1ef28b4f7c
Empact:
ACK 1ef28b4f7c
Tree-SHA512: 22ba4234985c6f9c1445b14565c71268cfaa121c4ef000ee3d5117212b09442dee8d46d9701bceddaf355263fe25dfe40def2ef614d4f2fe66c9ce876cb49934
this also fixes a bug that was uncovered with this checks:
for the BIP112 version 1 tx tests, certain txs (bip112txs_vary_OP_CSV_v1) have
been sent twice due to a typo, leading also to a failure as expected but for the
wrong reason
With BIP112 activated, the operation OP_CHECKSEQUENCEVERIFY (former OP_NOP3)
leads to script interpreter termination with an error if one of the following
conditions is true:
-> stack is empty
-> top item on stack is negative (< 0)
-> top item on stack has disable flag unset and at least one of
four other conditions is true (contains the core CSV logic)
This commits adds the missing empty stack failure test to the functional test
by prepending a valid scriptSig with just OP_CHECKSEQUENCEVERIFY. If BIP112 is
inactive, the operator just behaves as a NOP (for both tx versions 1 and 2) and
the transaction remains valid -- if it is active, the tx is invalid due to an
empty stack (for both tx versions 1 and 2, as well).
fa45d60646 test: Reduce unneeded whitelist permissions in tests (MarcoFalke)
Pull request description:
It makes the tests confusing and fragile when overwriting default command line values that are not needed to be overwritten.
ACKs for top commit:
fanquake:
ACK fa45d60646
laanwj:
ACK fa45d60646
Tree-SHA512: 8ae5ad8c6be156b1a983adccbca8d868ef841e00605ea88e24227f1b7493987c50b3e62e68dd7dc785ad73d6e14279eb13d7a151cb0a976426fe2fd63ce5cbcd
b902bd66b0 test: check custom descendant limit in mempool_packages.py (Sebastian Falbesoner)
Pull request description:
This is a follow-up PR to #17435, testing the custom descendant limit, passed by the argument `-limitdescendantcount`. ~~It was more tricky than expected, mainly because we don't know for sure at which point node1 has got all the transactions broadcasted from node0 (for the ancestor test this wasn't a problem since the txs were immediately available through `invalidateblock`) -- a simple `sync_mempools()` doesn't work here since the mempool contents are not equal due to different ancestor/descendant limits. Hence I came up with a "hacky manual sync":~~
1. ~~wait until the mempool has the _expected_ tx count (see conditions below)~~
2. ~~after that, wait some time and get sure that the mempool contents haven't changed in-between~~
~~Like for~~ Similar to the ancestor test, we overall check for ~~three~~ four conditions:
- the # of txs in the node1 mempool is equal to the descendant limit (plus 1 for the parent tx, plus the # txs from the previous ancestor test which are still in) ~~(done by the hacky sync above)~~
- all txs in node1 mempool are a subset of txs in node0 mempool
- part of the constructed descendant-chain (the first ones up to the limit) are contained in node1 mempool
- the remaining part of the constructed descendant-chain (all after the first ones up to the limit) is *not* contained in node1 mempool
ACKs for top commit:
JeremyRubin:
Excellent. utACK b902bd6
Tree-SHA512: 7de96dd248f16ab740e178ac5b64b57ead18cdcf74adfe989709d215e4a67b6b6d20de22c48e885d5f2edc55caaddd44a4261e996c5c87687ceb6a47f1d1fdaf
To test the custom descendant limit on node1 (passed by the argument
-limitdescendantcount), we check for four conditions:
-> the # of txs in the node1 mempool is equal to the limit
(plus 1 for the parent tx, plus the # txs from the previous ancestor
test which are still in)
-> all txs in node1 mempool are a subset of txs in node0 mempool
-> part of the constructed descendant-chain (the first ones up to the
limit) are contained in node1 mempool
-> the remaining part of the constructed descendant-chain (all after the
first ones up to the limit) is *not* contained in node1 mempool
facb71576c net: Remove forcerelay of rejected txs (MarcoFalke)
Pull request description:
This removes the code that supposedly handled the forced relay of txs from a permissioned peer that were rejected from our mempool. The removal should be fine, because it is dead code for the following reasons:
* While `RelayTransaction` enqueues the inv for all peers, the inv is never processed because it can not be found in the mempool. See 4a07233076/src/net_processing.cpp (L3862-L3866)
* Even if the peers we intended to send the inv to can somehow reply with a getdata to the never-received inv, they won't receive the tx as a reply because it was never added to the "relay memory" (`mapRelay`)
The dead code is (obviously) untested: https://marcofalke.github.io/btc_cov/total.coverage/src/net_processing.cpp.gcov.html#2574
This feature was (intentionally or accidentally) removed in 4d8993b346, which was released in Bitcoin Core 0.13.0. So all currently supported versions of Bitcoin Core ship without this feature. I am not aware of any complaints about this feature or actual documented use-cases. So instead of reviving an unneeded feature, just remove the dead code.
ACKs for top commit:
hebasto:
ACK facb71576c, locally running the unit and functional tests.
Tree-SHA512: bfceae6f2983c1510fa0649a9a63c343cbbc1c4ab3a3698039cccf454c81e58c8f5114b147ed42a1bc867da74c43a5b53764ab14f942e191b6f59079044108b5
5bad7921d0 [test] PSBT RPC: check that bip32_derivs are present by default (Sjors Provoost)
29a21c9061 [rpc] set default bip32derivs to true for psbt methods (Sjors Provoost)
Pull request description:
In https://github.com/bitcoin/bitcoin/pull/13557#pullrequestreview-135905054 I recommended not including bip32 deriviation by default in PSBTs:
> _Bit of a privacy issue_: let's say person A and B are about to spend from a multisig address, sending everything to person A. Person A gives their address to person B, their wallet wallet creates a PSBT, but doesn't sign it. Wallet A then calls `walletprocesspsbt` which signs it and _spontaneously adds the master_fingerprint and bip32 path_. Same issue with `walletcreatefundedpsbt`.
>
> Adding `bip32_derivs` should probably be opt-in.
In practice I find this default quite annoying because I forget it and end up with a confused hardware wallet.
More importantly, in the multisig example I provided, it's actually essential for the other side to know the derivation details (in addition to an xpub). This allows them to check that change is going to an address you can still co-sign for (because the spending policy is unchanged except for an index).
ACKs for top commit:
instagibbs:
utACK 5bad7921d0
jonatack:
ACK 5bad7921d0 code review, built, ran tests, inspected/messed around with/pprinted values from the new tests. Thanks for adding the tests.
meshcollider:
utACK 5bad7921d0
Tree-SHA512: 22ad71dda96856060a96758c4ae7aafa22d5e9efba30e0c8287c711e7579849bd72593cbc0f41a2e9e8821315d78bda04e848dbb006283b841b2795e2faebcfd
d6d2602a32 add: test that transactions expire from mempool (0xb10c)
Pull request description:
This adds the functional test `mempool_expiry.py` covering mempool transaction expiry. Both the default `DEFAULT_MEMPOOL_EXPIRY` of 336 hours (two weeks, set in #9312) and the user definable mempool expiry via the `-mempoolexpiry=<n>` command line option are tested. The test checks that descendants of expired transactions are removed as well.
*Notes for reviewers*
- `LimitMempoolSize()` (which is the only caller of `CTxMemPool::Expire()`) is only called when a transaction is added to the mempool. In order to test expiry of a transaction-that-should-expire, the mocktime is set and a random transaction is broadcast to trigger `LimitMempoolSize()`. The transaction-that-should-expire is then checked for expiry. LMK if there is another way, but I don't think there is.
ACKs for top commit:
MarcoFalke:
ACK d6d2602a32
theStack:
ACK d6d2602a32
promag:
Code review ACK d6d2602a32.
Tree-SHA512: eb68cd9e2d870872b8e8e1522fed8954fb99cc9e4edda4b28bb2a4e41cddbc53fe6f7d9c090f1e0e98ab49beb24bf37ff3787a9e9801a95e8ae9ca9eb34fe6f0
f2472f6460 tests: Improve test runner output in case of target errors (practicalswift)
733bbec34f tests: Add --exclude integer,parse_iso8601 (temporarily) to make Travis pass until uninitialized read issue in FormatISO8601DateTime is fixed (practicalswift)
5ea81449f3 tests: Add support for excluding fuzz targets using -x/--exclude (practicalswift)
555236f769 tests: Remove -detect_leaks=0 from test/fuzz/test_runner.py - no longer needed (practicalswift)
a3b539a924 ci: Run fuzz testing test cases under valgrind (practicalswift)
Pull request description:
Run fuzz testing [test cases (bitcoin-core/qa-assets)](https://github.com/bitcoin-core/qa-assets) under `valgrind`.
This would have caught `util: Avoid potential uninitialized read in FormatISO8601DateTime(int64_t) by checking gmtime_s/gmtime_r return value` (#18162) and similar cases.
ACKs for top commit:
MarcoFalke:
ACK f2472f6460👼
Tree-SHA512: bb0879d40167cf6906bc0ed31bed39db83c39c7beb46026f7b0ee53f28ff0526ad6fabc3f4cb3f5f18d3b8cafdcbf5f30105b35919f4e83697c71e838ed71493
This tests that a mempool transaction expires after a given timeout
and its children are removed as well.
Both the default expiry timeout defied by DEFAULT_MEMPOOL_EXPIRY and
a user definable expiry timeout via the -mempoolexpiry=<n> command
line argument (<n> is the timeout in hours) are tested.
a304a3632f Revert "Store p2sh scripts in AddAndGetDestinationForScript" (Russell Yanofsky)
eb7d8a5b07 [test] check for addmultisigaddress regression (Sjors Provoost)
005f8a92cc wallet: Improve LegacyScriptPubKeyMan::CanProvide script recognition (Russell Yanofsky)
Pull request description:
Make `LegacyScriptPubKeyMan::CanProvide` method able to recognize p2sh scripts when the redeem script is present in the `mapScripts` map without the p2sh script also having to be added to the `mapScripts` map. This restores behavior prior to #17261, which I think broke backwards compatibility with old wallet files by no longer treating addresses created by `addmultisigaddress` calls before #17261 as solvable.
The reason why tests didn't fail with the CanProvide implementation in #17261 is because of a workaround added in 4a7e43e846 "Store p2sh scripts in AddAndGetDestinationForScript", which masked the problem for new `addmultisigaddress` RPC calls without fixing it for multisig addresses already created in old wallet files.
This change adds a lot of comments and allows reverting commit 4a7e43e846 "Store p2sh scripts in AddAndGetDestinationForScript", so the `AddAndGetDestinationForScript()` function, `CanProvide()` method, and `mapScripts` map should all be more comprehensible
ACKs for top commit:
Sjors:
re-ACK a304a3632f (rebase, slight text changes and my test)
achow101:
re-ACK a304a3632f
meshcollider:
utACK a304a3632f
Tree-SHA512: 03b625220c49684c376a8062d7646aeba0e5bfe043f977dc7dc357a6754627d594e070e4d458d12d2291888405d94c1dbe08c7787c318374cedd5755e724fb6e
4e9efac678 test: Check wallet name in -walletnotify script (João Barbosa)
9a5b5ee81f wallet: Replace %w by wallet name in -walletnotify script (João Barbosa)
Pull request description:
Fixes#13237.
ACKs for top commit:
laanwj:
ACK 4e9efac678
Tree-SHA512: 189dd1c785485f2e974d7c12531851b2a977778b3b954aa95efd527322ba3345924cfd587fb9c90b0fa979202af0ab2d90e53d125fe266a36c94f757e4176203
5f26855f10 test: Remove ubsan alignment suppressions (Wladimir J. van der Laan)
9d933ef919 prevector: avoid misaligned member accesses (Anthony Towns)
Pull request description:
Ensure prevector data is appropriately aligned. Earlier discussion in #17530.
**Edit laanwj**: In contrast to #17530, it does this without increase in size of any of the coin cache data structures (x86_64, clang)
| Struct | (size,align) before | (size,align) after |
| ------------- | ------------- | ------- |
| Coin | 48, 8 | 48, 8 |
| CCoinsCacheEntry | 56, 8 | 56, 8 |
| CScript | 32, 1 | 32, 8 |
ACKs for top commit:
laanwj:
ACK 5f26855f10
practicalswift:
ACK 5f26855f10
jonatack:
ACK 5f26855f10
Tree-SHA512: 98d112d6856f683d5b212410b73f3071d2994f1efb046a2418a35890aa1cf1aa7c96a960fc2e963fa15241e861093c1ea41951cf5b4b5431f88345eb1dd0a98a
677fb8e923 test: Add ubsan surpression for crc32c (Wladimir J. van der Laan)
8e68bb1dde build: Disable msvc warning 4722 for leveldb build (Aaron Clauson)
be23949765 build: MSVC changes for leveldb update (Aaron Clauson)
9ebdf04757 build: CRC32C build system integration (Wladimir J. van der Laan)
402252a808 build: Add LCOV exception for crc32c (Wladimir J. van der Laan)
3a037d0067 test: Add crc32c exception to various linters and generation scripts (Wladimir J. van der Laan)
84ff1b2076 test: Add crc32c to subtree check linter (Wladimir J. van der Laan)
7cf13a5134 doc: Add crc32c subtree to developer notes (Wladimir J. van der Laan)
24d02a9ac0 build: Update build system for new leveldb (Wladimir J. van der Laan)
2e1819311a Squashed 'src/crc32c/' content from commit 224988680f7673cd7c769963d4035cb315aa3388 (Wladimir J. van der Laan)
66480821b3 Squashed 'src/leveldb/' changes from f545dfabff4c2e9836efed094dba99a34fbc6b88..f8ae182c1e5176d12e816fb2217ae33a5472fdd7 (Wladimir J. van der Laan)
Pull request description:
This updates leveldb to currently newest upstream commit 0c40829872:
- CRC32C hardware acceleration is now an external library [crc32c](https://github.com/google/crc32c). This adds acceleration on ARM, and should be faster on x86 because of using prefetch. It also makes it easy to support similar instruction sets on other platforms in the future.
- Thread handling uses C++11, instead of platform specific code.
- Native windows environment was added. No need to maintain our own hacky one, anymore.
- Upstream now builds using CMake. This doesn't mean we need to use that (phew), but internal configuration changed to a a series of checks, instead of OS profiles. This means the blanket error "Cannot build leveldb for $host. Please file a bug report' is removed.
All changes: a53934a3ae...0c40829872
Pretty much all our changes have been subsumed by upstream, so we figured it was cleaner to start over with a new branch from upstream with the still-relevant patches applied: https://github.com/bitcoin-core/leveldb/tree/bitcoin-fork-new
There's quite some testing to be done (see below). See https://github.com/bitcoin-core/leveldb/issues/25 and https://github.com/bitcoin-core/leveldb/pull/26 for more history and context.
TODO:
- [x] Subtree `crc32c`
- [x] Make linters happy about crc32 subtree
- [x] Integrate `crc32c` library into build system
- [x] MSVC build system
ACKs for top commit:
sipa:
ACK 677fb8e923
Tree-SHA512: 37ee92a750e053e924bc4626b12bb3fd81faa9f8c5ebaa343931fee810c45ba05aa6051fdea82535fa351bf2be7297801b98af9469865fc5ead771650a5d6240
19a354b11f Output a descriptor in createmultisig and addmultisigaddress (Andrew Chow)
Pull request description:
Give a descriptor from `createmultisig` and `addmultisigaddress`.
Extracted from #16528 with `addmultisgaddress` and tests added.
ACKs for top commit:
Sjors:
tACK 19a354b11f
MarcoFalke:
ACK 19a354b11f
promag:
Code review ACK 19a354b11f.
meshcollider:
utACK 19a354b11f
Tree-SHA512: e813125fbbc358ea8d45b1748de16a29a94efd83175b748fb8fa3b0bfc8e783ed36b6c554d84f5d4ead1ba252a83a3e937b6c3f75da7b8d3b4e55f94d6013771
900d8f6f70 util: Disallow network-qualified command line options (Russell Yanofsky)
Pull request description:
Previously these were allowed but ignored.
This change implements one of the settings simplifications listed in #17508. Change includes release notes.
ACKs for top commit:
laanwj:
ACK 900d8f6f70
Tree-SHA512: ab020a16a86c1e8ec709fbf798d533879d32c565eceeb7eb785c33042c49c6b4d1108c5453d8166e4a2abffc2c8802fbb6d3b895e0ddeefa8f274fd647e3c8ad
eca56f8929 test: replace 'regtest' leftovers by self.chain (Sebastian Falbesoner)
Pull request description:
This is a follow-up PR to #16681 (fixes#18068), replacing all remaining hardcoded `"regtest"` strings in functional tests by `self.chain`.
Top commit has no ACKs.
Tree-SHA512: 96524649b33164938e5a95215991103ed7855ebab55ef788d4816b3fa5cbc03d8f3b0d39f2247a87522f289fd7f4daf25e059900b8462b5127eb154bbee89054
1abcecc40c Tests: Use self.chain instead of 'regtest' in almost all current tests (Jorge Timón)
Pull request description:
Simply avoiding the hardcoded string in more places for consistency.
It can also allow for more easily reusing tests for other chains other than regtest.
Separated from #8994 .
Continues #16509 .
It is still not complete (ie to be complete, we need the -chain parameter in #16680 and make whether acceptnonstdtxs is allowed for that chain or not customizable for regtest [or for custom chains like in #8994 ] ). But while being incomplete like #16509 , it's quite simple to review and another step forward IMO.
ACKs for top commit:
Sjors:
re-ACK 1abcecc. I think it's an improvement even if incomplete and if some PR's might accidentally bring "regtest" back. Subsequent improvements hopefully don't have to touch 16 files.
elichai:
Code review ACK 1abcecc40c
ryanofsky:
Code review ACK 1abcecc40c.
ryanofsky:
Code review ACK 1abcecc40c
Tree-SHA512: 5620de6dab235ca8bd8670d6366c7b9f04f0e3ca9c5e7f87765b38e16ed80c17d7d1630c0d5fd7c5526f070830d94dc74cc2096d8ede87dc7180ed20569509ee
d3bc184081 doc: update release notes with getaddressinfo label deprecation (Jon Atack)
72af93f364 test: getaddressinfo label deprecation test (Jon Atack)
d48875fa20 rpc: deprecate getaddressinfo label field (Jon Atack)
dc0cabeda4 test: remove getaddressinfo label tests (Jon Atack)
c7654af6f8 doc: address pr17578 review feedback (Jon Atack)
Pull request description:
This PR builds on #17578 (now merged) and deprecates the rpc getaddressinfo `label` field. The deprecated behavior can be re-enabled by starting bitcoind with `-deprecatedrpc=label`.
See http://www.erisian.com.au/bitcoin-core-dev/log-2019-11-22.html#l-622 and https://github.com/bitcoin/bitcoin/pull/17283#issuecomment-554458001 for more context.
Reviewers: This PR may be tested manually by building, then running bitcoind with and without the `-deprecatedrpc=label` flag while verifying the rpc getaddressinfo output and help text.
Next step: add support for multiple labels.
ACKs for top commit:
jnewbery:
ACK d3bc184081
laanwj:
ACK d3bc184081
meshcollider:
utACK d3bc184081
Tree-SHA512: f954402884ec54977def332c8160fd892f289b0d2aee1e91fed9ac3220f7e5b1f7fc6421b84cc7a5c824a0582eca4e6fc194e4e33ddd378c733c8941ac45f56d
cb8a86d9f9 gui: Remove WalletView and BitcoinGUI circular dependency (João Barbosa)
ac3d10777d gui: Add transactionClicked and coinsSent signals to WalletView (João Barbosa)
Pull request description:
Essentially moves the code in `WalletView::setBitcoinGUI` to the only caller. Two new signals are added beforehand in the first commit so that the connections in `WalletFrame` are all from the wallet view.
ACKs for top commit:
hebasto:
ACK cb8a86d9f9, tested on Linux Mint 19.3.
jonasschnelli:
utACK cb8a86d9f9
Tree-SHA512: 250316cd3689e51c8cded9ccd75963c836dcafa6db25d684f2aa691dea9738895f9140793e0f925784909e39f8257f7e1c7d611e8bd6d6634e1a50333f4ddb1e
b951b0973c on startup, write config options to debug.log (Larry Ruane)
Pull request description:
When a developer is examining `debug.log` after something goes wrong, it's often useful to know the exact options the failing instance of `bitcoind` was started with. Sometimes the `debug.log` file is all that's available for the analysis. This PR logs the `bitcoin.conf` entries and command-line arguments to `debug.log` on startup.
ACKs for top commit:
MarcoFalke:
ACK b951b0973c🐪
jonatack:
ACK b951b0973c reviewed diff, re-code review, built, ran tests, launched bitcoind and reviewed debug log output, verified value of `str` debug log in the added unit test.
Tree-SHA512: bbca4fb3d49f99261758302bde0b8b67300ccc72e7380b01f1f66a146ae8a008a045df0ca5ca9664caff034d0ee38ea7ef38a50f38374525608c07ba52790358
