backport: bitcoin#20459, #20842, #21557, #21840, #21867, #21897, #21900, #21945, #22048, #22057 #6152
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 6 commits
July 26, 2024 13:32
fa8192f rpc: Fail to return undocumented return values (MarcoFalke) Pull request description: Currently a few return values are undocumented. This is causing confusion at the least. See for example bitcoin#18476 Fix this by treating it as an internal bug to return undocumented return values. ACKs for top commit: ryanofsky: Code review ACK fa8192f. Only changes: rebase, no const_cast suggestion, and tostring cleanups needed after suggestion Tree-SHA512: c006905639bafe3045de152b00c34d9864731becb3c4f468bdd61a392f10d7e7cd89a54862c8daa8c11ac4eea0eb5f13b0f647d21e21a0a797b54191cff7238c
BACKPORT NOTICE: missing changes in src/test/validation_tests.cpp (signet) 1112035 doc: fix various typos (Ikko Ashimine) e864084 doc: Use https URLs where possible (Sawyer Billings) Pull request description: Consolidates / fixes the changes from bitcoin#20762, bitcoin#20836, bitcoin#20810. There is no output when `test/lint/lint-all.sh` is run. Closes bitcoin#20807. ACKs for top commit: MarcoFalke: ACK 1112035 Tree-SHA512: 22ca824688758281a74e5ebc6a84a358142351434e34c88c6b36045d2d241ab95fd0958565fd2060f98317e62e683323b5320cc7ec13592bf340e6922294ed78
6526a16 test: small cleanup in RPCNestedTests tests (fanquake) Pull request description: Remove QtDir & QtGlobal (dea086f) Add missing includes. Remove obsolete comment about Qt 5.3 (fd46c4c) Top commit has no ACKs. Tree-SHA512: 097e603fc31a19be1817459ad4c5a9692708f8a39a0ae87e4a60eabc22bf4f6141b577ba68746044fd594f92e36848b7cd56d60dccd262f83f8ec7310ab7d1bc
9f767e8 test: use MiniWallet for p2p_blocksonly.py (Sebastian Falbesoner) Pull request description: This PR enables one more of the non-wallet functional tests (p2p_blocksonly.py) to be run even with the Bitcoin Core wallet disabled by using the new MiniWallet instead, as proposed in bitcoin#20078. Note that MiniWallet creates segwit transactions by default, i.e. txid and wtxid are not identical and we have to return both from `check_p2p_tx_violation(...)`: wtxid is needed to match an expected `"received getdata for: wtx ..."` debug output, whereas the txid is needed to wait for a certain tx via `wait_for_tx(...)`. ACKs for top commit: jonatack: ACK 9f767e8 tested with `--disable-wallet` Tree-SHA512: f08001f02c3c310ccdf713af0ba17304368a36414f412749908bbe8c03ad1e902190b8768b79f3b4909855762f285e7ab1b627cc4f45c90b42bb097a43cb4318
7031721 rpc/listaddressgroupings: redefine inner-most array as ARR_FIXED (Karl-Johan Alm) 8500f7b rpc/createrawtransaction: redefine addresses as OBJ_USER_KEYS (Karl-Johan Alm) d9e2183 rpc: include OBJ_USER_KEY in RPCArg constructor checks (Karl-Johan Alm) Pull request description: This PR adjusts the two issues I encountered while developing a tool that converts RPCHelpMan objects into bindings for other language(s). The first is in createrawtransaction, where the address part, e.g. bc1qabc in > createrawtransaction '[]' '[{"bc1qabc": 1.0}]' is declared as a `Type::OBJ`, when in reality it should be a `Type::OBJ_USER_KEYS`, defined as such: https://github.com/bitcoin/bitcoin/blob/5925f1e652768a9502831b9ccf78d16cf3c37d29/src/rpc/util.h#L126 (coincidentally, this is the first and only (afaict) usage of this `RPCArg::Type`). The second is in the `listaddressgroupings` RPC, which returns an array of arrays of arrays, where the innermost one is a tuple-thingie with an optional 3rd item; this is an `ARR_FIXED`, not an `ARR`. ACKs for top commit: MarcoFalke: ACK 7031721 🐀 Tree-SHA512: 769377416c6226d1738a956fb685498e009f9e7eb2d45bc679b81c5364b9520fdbcb49392c937ab45598aa0d33589e8e6a59ccc101cf8d8e7dfdafd58d4eefd0
bd7f27d refactor: feature_csv_activation.py: move tx helper functions to methods (Sebastian Falbesoner) 2eca46b test: use MiniWallet for feature_csv_activation.py (Sebastian Falbesoner) Pull request description: This PR enables one more of the non-wallet functional tests (feature_csv_activation.py) to be run even with the Bitcoin Core wallet disabled by using the new MiniWallet instead, as proposed in bitcoin#20078. Short reviewers guideline: - Since we exclusively work with anyone-can-spend outputs here (raw scriptPubKey = OP_TRUE), signing is not needed anymore. The function `sign_transaction` and its calls are removed, after changing a tx (e.g. its scriptSig or nVersion) a simple `.rehash()` call is sufficient. Also, generating an address `self.nodeaddress` (and with that, passing it to the the various test tx creation/sending helper methods) is not needed anymore and removed. - The test repeatedly uses the same input for creating different txs (e.g. with different txversions 1 and 2). To let `MiniWallet` create a tx with a specific input, we have to call `.get_utxo()` before which also marks the UTXO as spent. The method is changed to also support keeping the UTXO in its internal list (`mark_as_spent=False`). With the behaviour on master, the second call to `.get_utxo()` with the same input would fail. - To keep the diff in the first commit short, the `miniwallet` is set as a global variable, to avoid passing it on every tx creation/spending helper. The global is eliminated in the second (refactoring) commit, where all the helpers are moved to the test class as methods. By that, we can use `self.nodes[0]` directly in the helpers and don't have to pass it again and again. I think there could still be a lot of improvements/refactoring done in the test, but that should hopefully serve as a good basis. ACKs for top commit: laanwj: Code review ACK bd7f27d MarcoFalke: review ACK bd7f27d 🐕 Tree-SHA512: 24fb6a0f7702bae40d5271d197119827067d4b597e954d182e4c1aa5d0fa870368eb3ffed469b26713fa8ff8eb3ecc06abc80b2449cd68156d5559e7ae8a2b11
…inters fa8a888 bench: Remove duplicate constants (MarcoFalke) 000098f test: Use throwing variant accessor (MarcoFalke) fa2197c test: Use loop to register RPCs (MarcoFalke) Pull request description: Simplify test code ACKs for top commit: Empact: Code Review ACK fa8a888 practicalswift: cr ACK fa8a888 promag: Code review ACK fa8a888. Tree-SHA512: 6a5bebaa9a3f43e9c332f4fbff606e9ece6dc8b95a769980082cc022f8e9bde6083c1e4a0145dcbf3741f514d6e97b4198f201a1bf1370ebf43bd3a5c0f85981
It's a double size because 1 byte is 2 hex character, not because it is 'segwit'
4bea301 test: use P2PK-MiniWallet for feature_csv_activation.py (Sebastian Falbesoner) dc7eb64 test: MiniWallet: add P2PK support (Sebastian Falbesoner) Pull request description: This PR adds support for creating and spending transactions with raw pubkey (P2PK) outputs to MiniWallet, [as suggested by MarcoFalke](bitcoin#21900 (comment)). Using that mode in the test `feature_csv_activation.py`, all txs submitted to the mempool follow the standard policy, i.e. `-acceptnonstdtxn=1` can be removed. Possible follow-ups: * Improve MiniWallet constructor Interface; an enum-like parameter instead of two booleans would probably be better * Look at other tests that could benefit from P2PK (e.g. feature_cltv.py?) * Check vsize also for P2PK txs (vsize varies due to signature, i.e. a range has to be asserted) ACKs for top commit: laanwj: Code review ACK 4bea301 Tree-SHA512: 9b428e6b7cfde59a8c7955d5096cea88af1384a5f49723f00052e9884d819d952d20a5ab39bb02f9d8b6073769c44462aa265d84a33e33da33c2d21670c488a6
… mode 6cebac5 test: MiniWallet: introduce enum type for output mode (Sebastian Falbesoner) Pull request description: This is a follow-up PR to bitcoin#21945 which lifted the number of MiniWallet's tx output modes from 2 to 3 (by adding P2PK Support). Since the current way of specifying the mode on the ctor via two booleans is ugly and error-prone (see table in comment bitcoin#21945 (comment)), a new Enum type `MiniWalletMode` is introduced that can hold the following values: - ADDRESS_OP_TRUE - RAW_OP_TRUE - RAW_P2PK Also adds documentation that should guide the user on which mode is useful for what etc. with a summary table. (Can also be split up in a separate commit or shortened if that is desired, maybe it's considered to be too verbose). ACKs for top commit: MarcoFalke: cr ACK 6cebac5 Tree-SHA512: cbbc10806d9d9e62829548094415e9f1a281cd247b9a9fc7f7f33b923c723aa03e7bc3024623a77fb1f7da4d73455fa8244840f746980d32acdad97ee12100da
…sig.py 3e05a57 test: use MiniWallet (P2PK mode) for feature_dersig.py (Sebastian Falbesoner) Pull request description: This PR enables one more of the non-wallet functional tests (feature_dersig.py) to be run even with the Bitcoin Core wallet disabled. A valid DER-signature is created by using the recently introduced P2PK-Mode of the MiniWallet (bitcoin#21945). ACKs for top commit: MarcoFalke: cr ACK 3e05a57 Tree-SHA512: 0fb8da8ed8b47f68bcb57301eb4f0171a6c9e44539b7554626969347e5d6f80b3b9085f2cc160cd038a990f0d81b8b614846260fbed43b5f950d77f1b7aa81cf
UdjinM6
approved these changes
Jul 26, 2024
Comment on lines
14
to
15
| #include <llmq/context.h> | ||
|
|
Collaborator
Author
There was a problem hiding this comment.
That's missing changes from #6112 - caught one more case but 6112 is merged already
Member
There was a problem hiding this comment.
would've preferred in separate PR, but this is fine
5 tasks
PastaPastaPasta
added a commit
that referenced
this pull request
Oct 21, 2024
, bitcoin#24626, bitcoin#21726, bitcoin#25123, bitcoin#25074, bitcoin#24832, bitcoin#26215, bitcoin#24858, bitcoin#26417, bitcoin#16981 (index backports) 7d9ff96 merge bitcoin#16981: Improve runtime performance of --reindex (Kittywhiskers Van Gogh) e531dff merge bitcoin#26417: fix intermittent failure in feature_index_prune.py (Kittywhiskers Van Gogh) b04b71a merge bitcoin#24858: incorrect blk file size calculation during reindex results in recoverable blk file corruption (Kittywhiskers Van Gogh) 9e75b99 merge bitcoin#26215: Improve BaseIndex::BlockUntilSyncedToCurrentChain reliability (Kittywhiskers Van Gogh) 3bd584c merge bitcoin#24832: Verify the block filter hash when reading the filter from disk (Kittywhiskers Van Gogh) e507a51 fix: avoid `mandatory-script-verify-flag-failed` crash in bench test (Kittywhiskers Van Gogh) a86109a merge bitcoin#25074: During sync, commit best block after indexing (Kittywhiskers Van Gogh) e6867a3 merge bitcoin#25123: Fix race condition in index prune test (Kittywhiskers Van Gogh) baf6e26 merge bitcoin#21726: Improve Indices on pruned nodes via prune blockers (Kittywhiskers Van Gogh) c65ec19 merge bitcoin#24626: disallow reindex-chainstate when pruning (Kittywhiskers Van Gogh) bcd24a2 fix: push activation height for forks ahead, fix `feature_pruning.py` (Kittywhiskers Van Gogh) 1020356 merge bitcoin#24812: Add CHECK_NONFATAL identity function and NONFATAL_UNREACHABLE macro (Kittywhiskers Van Gogh) 1caaa85 merge bitcoin#24138: Commit MuHash and best block together for coinstatsindex (Kittywhiskers Van Gogh) b218f12 merge bitcoin#23046: Add txindex migration test (Kittywhiskers Van Gogh) ebae59e fix: make sure we flush our committed best block in no-upgrade cases (Kittywhiskers Van Gogh) Pull request description: ## Additional Information * When backporting [bitcoin#23046](bitcoin#23046), it was discovered that there has been a longstanding bug in `CDeterministicMNManager::MigrateDBIfNeeded`(`2`)`()` that flagged a database taken from an older version for failing its "previous migration attempt", requiring the database to be fully rebuilt through a reindex. This occurred because the older database would be read pre-DIP3 in `MigrateDBIfNeeded()`, which then caused the migration logic to write the new best block ([source](https://github.com/dashpay/dash/blob/3f0c2ff324fde73bcf3d85c68ce5200d73987eda/src/evo/deterministicmns.cpp#L1236-L1241)) (the legacy best block is erased before the DIP3 condition is checked, [source](https://github.com/dashpay/dash/blob/3f0c2ff324fde73bcf3d85c68ce5200d73987eda/src/evo/deterministicmns.cpp#L1233)) but while it completed the transaction ([source](https://github.com/dashpay/dash/blob/3f0c2ff324fde73bcf3d85c68ce5200d73987eda/src/evo/deterministicmns.cpp#L1240)), critically, it didn't write it to disk (example of writing to disk, [here](https://github.com/dashpay/dash/blob/3f0c2ff324fde73bcf3d85c68ce5200d73987eda/src/evo/deterministicmns.cpp#L1288-L1292)). This meant that when it was read again by `MigrateDBIfNeeded2()`, it saw three things a) there is no new best block (because it didn't get written), b) there is no legacy best block (because it gets erased before the new best block is written) and c) that the chain height is greater than 1 (since this isn't a new datadir and the chain has already advanced), it concludes that it was a botched migration attempt and fails ([source](https://github.com/dashpay/dash/blob/3f0c2ff324fde73bcf3d85c68ce5200d73987eda/src/evo/deterministicmns.cpp#L1337-L1343)). This bug affects v19 to `develop` (`3f0c2ff3` as of this writing) and prevents `feature_txindex_compatibility.py` from working as expected as it would migrate legacy databases to newer versions to test txindex migration code and get stuck at unhappy EvoDB migration logic, to allow the test to function properly when testing against the latest version of the client, this bug has been fixed as part of this PR. * In [bitcoin#23046](bitcoin#23046), version v0.17 was used as the last version to support legacy txindex as the updated txindex format was introduced in [dash#4178](#4178) (i.e. after v0.17) and the version selected for having migration code in it (note, migration code was removed in [dash#6296](#6296), so far not included as part of any release) was v18.2.2 despite the range being v18.x to v21.x was a) due to the bug mentioned above affecting v19.x onwards and b) v18.2.2 being the latest release in the v18.x lifecycle. * The specific version number used for v0.17 is `170003` as the binaries corresponding to `170000` are not populated in `releases/`, which causes a CI failure ([source](https://gitlab.com/dashpay/dash/-/jobs/8073041955#L380)) * As of `develop` (`3f0c2ff3` as of this writing), `feature_pruning.py` was broken due to changes in Core that were not adjusted for, namely: * The enforcement of `MAX_STANDARD_TX_SIZE` ([source](https://github.com/dashpay/dash/blob/3f0c2ff324fde73bcf3d85c68ce5200d73987eda/src/policy/policy.h#L23-L24)) from DIP1 onwards ([source](https://github.com/dashpay/dash/blob/3f0c2ff324fde73bcf3d85c68ce5200d73987eda/src/validation.cpp#L299-L301)) resulting in `bad-txns-oversize` errors in blocks generated for the test as the transactions generated are ~9.5x larger than the now-enforced limit ([source](https://github.com/dashpay/dash/blob/3f0c2ff324fde73bcf3d85c68ce5200d73987eda/test/functional/feature_pruning.py#L48C51-L48C57)), this is resolved by pushing the DIP1 activation height upwards to `2000` (the same activation height used for DIP3 and DIP8). * Change in subsidy logic in v20 ([source](https://github.com/dashpay/dash/blob/3f0c2ff324fde73bcf3d85c68ce5200d73987eda/src/validation.cpp#L1073-L1082)) that results in `bad-cb-amount` errors, this is resolved by pushing the v20 activation height upwards. Additionally, an inopportune implicit post-`generate` sync ([source](https://github.com/dashpay/dash/blob/3f0c2ff324fde73bcf3d85c68ce5200d73987eda/test/functional/feature_pruning.py#L215)) also causes the test to fail. Alongside the above, they have been resolved in this PR. * As of `develop` (`3f0c2ff3` as of this writing), `bench_dash` crashes when running the `AssembleBlock` benchmark. The regression was traced back to [bitcoin#21840](bitcoin#21840) (5d10b41) in [dash#6152](#6152) due to the switch to `P2SH_OP_TRUE`. This has been resolved by reverting this particular change. <details> <summary>Pre-fix test failure:</summary> ``` $ ./src/bench/bench_dash Warning, results might be unstable: * CPU governor is '' but should be 'performance' * Turbo is enabled, CPU frequency will fluctuate Recommendations * Use 'pyperf system tune' before benchmarking. See https://github.com/psf/pyperf | ns/op | op/s | err% | ins/op | bra/op | miss% | total | benchmark |--------------------:|--------------------:|--------:|----------------:|---------------:|--------:|----------:|:---------- | 17,647,657.00 | 56.66 | 0.1% | 231,718,349.00 | 42,246,265.00 | 0.1% | 0.20 | `AddrManAdd` | 42,201,861.00 | 23.70 | 0.1% | 544,366,811.00 | 102,569,244.00 | 0.0% | 0.46 | `AddrManAddThenGood` | 189,697.83 | 5,271.54 | 0.1% | 1,763,991.40 | 356,189.40 | 0.3% | 0.01 | `AddrManGetAddr` | 454.38 | 2,200,808.04 | 0.6% | 6,229.11 | 1,343.92 | 0.1% | 0.01 | `AddrManSelect` | 1,066,471.00 | 937.67 | 67.6% | 13,350,463.00 | 3,150,465.00 | 0.4% | 0.01 | 〰️ `AddrManSelectByNetwork` (Unstable with ~1.0 iters. Increase `minEpochIterations` to e.g. 10) | 1,181,774.50 | 846.19 | 49.0% | 18,358,489.50 | 4,224,642.50 | 0.0% | 0.02 | 〰️ `AddrManSelectFromAlmostEmpty` (Unstable with ~1.1 iters. Increase `minEpochIterations` to e.g. 11) bench_dash: bench/block_assemble.cpp:46: void AssembleBlock(benchmark::Bench &): Assertion `res.m_result_type == MempoolAcceptResult::ResultType::VALID' failed. [1] 2343746 IOT instruction (core dumped) ./src/bench/bench_dash ``` </details> ## Breaking changes None expected ## Checklist - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [x] I have added or updated relevant unit/integration/functional/e2e tests - [x] I have made corresponding changes to the documentation **(note: N/A)** - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: utACK 7d9ff96 PastaPastaPasta: utACK 7d9ff96 Tree-SHA512: e2f1e58abb0a0368c4f1d5e488520957e042e6207b7d2d68a15eb18662405a3cdac91c5ff8e93c8a94c0fdab9b1412bd608d055f196230506c1640439939c25d
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What was done?
Backports from v22 bitcoin.
Mostly related to MiniWallet and RPC improvements, see commits
How Has This Been Tested?
Run unit/functional tests
Breaking Changes
N/A
Checklist: