📝 nervana21 opened a pull request: "validation: Keep chain tip in candidate set"
(https://github.com/bitcoin/bitcoin/pull/33130)
After `PruneBlockIndexCandidates()` is called, the candidate set must include the current chain tip or a successor of it.
`PruneBlockIndexCandidates()` removes candidates with less work than the active tip. However, because the tip itself is never added to `setBlockIndexCandidates` via `TryAddBlockIndexCandidate()`, it may be absent from the set. Prior to this patch, if all lower-work candidates were pruned and the tip was not present, `setBlockIndexCandidates` could become empty—violating an
...
(https://github.com/bitcoin/bitcoin/pull/33130)
After `PruneBlockIndexCandidates()` is called, the candidate set must include the current chain tip or a successor of it.
`PruneBlockIndexCandidates()` removes candidates with less work than the active tip. However, because the tip itself is never added to `setBlockIndexCandidates` via `TryAddBlockIndexCandidate()`, it may be absent from the set. Prior to this patch, if all lower-work candidates were pruned and the tip was not present, `setBlockIndexCandidates` could become empty—violating an
...
💬 nervana21 commented on pull request "validation: Keep chain tip in candidate set":
(https://github.com/bitcoin/bitcoin/pull/33130#issuecomment-3148457986)
cc: @Ataraxia009
(https://github.com/bitcoin/bitcoin/pull/33130#issuecomment-3148457986)
cc: @Ataraxia009
📝 cedwies opened a pull request: "tests: cover abortrescan() in-flight True path with dynamic-tail retry"
(https://github.com/bitcoin/bitcoin/pull/33131)
This PR adds a test in wallet_transactiontime_rescan.py. Previously we only tested that abortrescan() returns False when no rescan was running. This change verifies that it returns True and actually interrupts a running scan.
How it works:
1. spawn a daemon background thread that rescans a tail of the wallet’s chain.
2. aggressively poll every 10 ms (for up to 5 s) until we see the node start scanning
3. Call 'abortrescan()' and assert to True
4. Threads are joined with short timeouts so
...
(https://github.com/bitcoin/bitcoin/pull/33131)
This PR adds a test in wallet_transactiontime_rescan.py. Previously we only tested that abortrescan() returns False when no rescan was running. This change verifies that it returns True and actually interrupts a running scan.
How it works:
1. spawn a daemon background thread that rescans a tail of the wallet’s chain.
2. aggressively poll every 10 ms (for up to 5 s) until we see the node start scanning
3. Call 'abortrescan()' and assert to True
4. Threads are joined with short timeouts so
...
🤔 cedwies reviewed a pull request: "test: refactor mempool_accept_wtxid"
(https://github.com/bitcoin/bitcoin/pull/33067#pullrequestreview-3082223310)
Changes look great.
Nice to see the valid_witness_malleate_tx helper instead of the ValidWitnessMalleatedTx class. Makes the test's logic much clearer IMO.
ACK 0cbcdca
(https://github.com/bitcoin/bitcoin/pull/33067#pullrequestreview-3082223310)
Changes look great.
Nice to see the valid_witness_malleate_tx helper instead of the ValidWitnessMalleatedTx class. Makes the test's logic much clearer IMO.
ACK 0cbcdca
💬 ismaelsadeeq commented on pull request "util: detect and warn when using exFAT on MacOS":
(https://github.com/bitcoin/bitcoin/pull/31453#issuecomment-3148513933)
It will be nice if this get in to v30, cc all previous reviewers @maflcko perhaps add the v30.0 milestone
(https://github.com/bitcoin/bitcoin/pull/31453#issuecomment-3148513933)
It will be nice if this get in to v30, cc all previous reviewers @maflcko perhaps add the v30.0 milestone
💬 Ataraxia009 commented on issue "Crash when chain tip is missing from candidate set":
(https://github.com/bitcoin/bitcoin/issues/33129#issuecomment-3148515467)
The issue I was addressing has nothing to do with `FindMostWorkChain` or a crash in it.
The root issue that I witnessed was a crash on launch in `PruneBlockIndexCandidates` which is caused by the data dir reaching a bad state.
In this state, the block validity of the block indexes in the `setBlockIndexCandidates` was correct, i.e.: `BLOCK_VALID_TRANSACTIONS`,
but the block validity for the blocks that were used to to build the `CoinsView` and therefore the `M_Chain` did not have their require
...
(https://github.com/bitcoin/bitcoin/issues/33129#issuecomment-3148515467)
The issue I was addressing has nothing to do with `FindMostWorkChain` or a crash in it.
The root issue that I witnessed was a crash on launch in `PruneBlockIndexCandidates` which is caused by the data dir reaching a bad state.
In this state, the block validity of the block indexes in the `setBlockIndexCandidates` was correct, i.e.: `BLOCK_VALID_TRANSACTIONS`,
but the block validity for the blocks that were used to to build the `CoinsView` and therefore the `M_Chain` did not have their require
...
💬 furszy commented on issue "Crash when chain tip is missing from candidate set":
(https://github.com/bitcoin/bitcoin/issues/33129#issuecomment-3148522739)
Which version of Bitcoin-Core are you running?
(https://github.com/bitcoin/bitcoin/issues/33129#issuecomment-3148522739)
Which version of Bitcoin-Core are you running?
🤔 mzumsande reviewed a pull request: "validation: Keep chain tip in candidate set"
(https://github.com/bitcoin/bitcoin/pull/33130#pullrequestreview-3082372722)
> After PruneBlockIndexCandidates() is called, the candidate set must include the current chain tip or a successor of it.
`setBlockIndexCandidates` must at all times include all connectable blocks that have at least as much work as the tip, so the tip itself must at all times be in the set.
> However, because the tip itself is never added to setBlockIndexCandidates via TryAddBlockIndexCandidate(), it may be absent from the set.
This seems wrong / makes no sense to me. No block can ever
...
(https://github.com/bitcoin/bitcoin/pull/33130#pullrequestreview-3082372722)
> After PruneBlockIndexCandidates() is called, the candidate set must include the current chain tip or a successor of it.
`setBlockIndexCandidates` must at all times include all connectable blocks that have at least as much work as the tip, so the tip itself must at all times be in the set.
> However, because the tip itself is never added to setBlockIndexCandidates via TryAddBlockIndexCandidate(), it may be absent from the set.
This seems wrong / makes no sense to me. No block can ever
...
💬 mzumsande commented on issue "OpenBSD, NetBSD: `-reindex` is broken":
(https://github.com/bitcoin/bitcoin/issues/33128#issuecomment-3148775143)
Does this mean that `feature_reindex.py` still succeeds, so that reindex only fails if we delete the `index/` dir like in `feature_reindex_init.py`, but not if we just reindex without corrupting anything?!
(https://github.com/bitcoin/bitcoin/issues/33128#issuecomment-3148775143)
Does this mean that `feature_reindex.py` still succeeds, so that reindex only fails if we delete the `index/` dir like in `feature_reindex_init.py`, but not if we just reindex without corrupting anything?!
💬 fjahr commented on pull request "index: Fix coinstats overflow":
(https://github.com/bitcoin/bitcoin/pull/30469#discussion_r2250201595)
Ok, I am working on a PR for this and I should have it ready by tomorrow but I think it's better to keep it separate from this one.
(https://github.com/bitcoin/bitcoin/pull/30469#discussion_r2250201595)
Ok, I am working on a PR for this and I should have it ready by tomorrow but I think it's better to keep it separate from this one.
💬 fjahr commented on pull request "index: Fix coinstats overflow":
(https://github.com/bitcoin/bitcoin/pull/30469#issuecomment-3148815473)
> which I think could be enabled for coinstatsindex relatively easily, FYI @furszy
Not sure if I maybe misunderstand that part but do you mean parallelization could be enabled easily for coinstatsindex? I haven't reviewed https://github.com/bitcoin/bitcoin/pull/26966 in depth yet but I don't think that's the case because the blocks have to be processed in order.
(https://github.com/bitcoin/bitcoin/pull/30469#issuecomment-3148815473)
> which I think could be enabled for coinstatsindex relatively easily, FYI @furszy
Not sure if I maybe misunderstand that part but do you mean parallelization could be enabled easily for coinstatsindex? I haven't reviewed https://github.com/bitcoin/bitcoin/pull/26966 in depth yet but I don't think that's the case because the blocks have to be processed in order.
✅ nervana21 closed a pull request: "validation: Keep chain tip in candidate set"
(https://github.com/bitcoin/bitcoin/pull/33130)
(https://github.com/bitcoin/bitcoin/pull/33130)
💬 nervana21 commented on pull request "validation: Keep chain tip in candidate set":
(https://github.com/bitcoin/bitcoin/pull/33130#issuecomment-3148820941)
Hi mzumsande,
Thanks for the thorough feedback.
You're right, it is preferable to crash a node that is out of consensus. I believe that I did have a chain sequence construction that led to `setBlockIndexCandidates` without chain tip (now I'm less confident), but even so, I'm certain the chain construction would be out of consensus and therefore not worth salvaging. I wish I had considered before and I will try to consider in the future.
Thanks again!
(https://github.com/bitcoin/bitcoin/pull/33130#issuecomment-3148820941)
Hi mzumsande,
Thanks for the thorough feedback.
You're right, it is preferable to crash a node that is out of consensus. I believe that I did have a chain sequence construction that led to `setBlockIndexCandidates` without chain tip (now I'm less confident), but even so, I'm certain the chain construction would be out of consensus and therefore not worth salvaging. I wish I had considered before and I will try to consider in the future.
Thanks again!
📝 theStack opened a pull request: "fuzz: txgraph: fix `real_is_optimal` flag propagation in `CommitStaging`"
(https://github.com/bitcoin/bitcoin/pull/33132)
In the `txgraph` fuzz test, the `CommitStaging` step updates the `SimTxGraph` levels simply by erasing the front (=main) one in the `sims` vector, i.e. the staging level instance takes the place of the main level instance:
https://github.com/bitcoin/bitcoin/blob/83a2216f5278c1123ba740f1c8a055652e033ddd/src/test/fuzz/txgraph.cpp#L668-L672
This also includes the `real_is_optimal` flag (reflecting whether the corresponding real graph is known to be optimally linearized), without taking into a
...
(https://github.com/bitcoin/bitcoin/pull/33132)
In the `txgraph` fuzz test, the `CommitStaging` step updates the `SimTxGraph` levels simply by erasing the front (=main) one in the `sims` vector, i.e. the staging level instance takes the place of the main level instance:
https://github.com/bitcoin/bitcoin/blob/83a2216f5278c1123ba740f1c8a055652e033ddd/src/test/fuzz/txgraph.cpp#L668-L672
This also includes the `real_is_optimal` flag (reflecting whether the corresponding real graph is known to be optimally linearized), without taking into a
...
💬 theStack commented on issue "OpenBSD, NetBSD: `-reindex` is broken":
(https://github.com/bitcoin/bitcoin/issues/33128#issuecomment-3148839416)
> [@theStack](https://github.com/theStack)
>
> Can you confirm the bug?
Will test in 1-2 weeks, as I unfortunately don't have access to an OpenBSD machine right now.
(https://github.com/bitcoin/bitcoin/issues/33128#issuecomment-3148839416)
> [@theStack](https://github.com/theStack)
>
> Can you confirm the bug?
Will test in 1-2 weeks, as I unfortunately don't have access to an OpenBSD machine right now.
💬 Crypt-iQ commented on pull request "validation: detect witness stripping early on":
(https://github.com/bitcoin/bitcoin/pull/33105#issuecomment-3148912384)
I'm a little out of my depth and it is getting late, but reviewing this PR I think it's a slight behavior change in what we see as witness-stripped? P2SH-P2WSH are not caught by `IsWitnessProgram` in this PR, but they are caught as witness-stripped in master. I checked by modifying the test_p2sh_witness subtest in p2p_segwit.py. I didn't test P2SH-P2WPKH Not sure if it matters, figured I'd point it out.
(https://github.com/bitcoin/bitcoin/pull/33105#issuecomment-3148912384)
I'm a little out of my depth and it is getting late, but reviewing this PR I think it's a slight behavior change in what we see as witness-stripped? P2SH-P2WSH are not caught by `IsWitnessProgram` in this PR, but they are caught as witness-stripped in master. I checked by modifying the test_p2sh_witness subtest in p2p_segwit.py. I didn't test P2SH-P2WPKH Not sure if it matters, figured I'd point it out.
🤔 furszy reviewed a pull request: "index: Fix coinstats overflow"
(https://github.com/bitcoin/bitcoin/pull/30469#pullrequestreview-3082638040)
> > which I think could be enabled for coinstatsindex relatively easily, FYI @furszy
>
> Not sure if I maybe misunderstand that part but do you mean parallelization could be enabled easily for coinstatsindex? I haven't reviewed #26966 in depth yet but I don't think that's the case because the blocks have to be processed in order.
It shouldn't take me much to parallelize it. Most fields here are essentially accumulators. Blocks can be read from disk and processed in parallel. Only the dump
...
(https://github.com/bitcoin/bitcoin/pull/30469#pullrequestreview-3082638040)
> > which I think could be enabled for coinstatsindex relatively easily, FYI @furszy
>
> Not sure if I maybe misunderstand that part but do you mean parallelization could be enabled easily for coinstatsindex? I haven't reviewed #26966 in depth yet but I don't think that's the case because the blocks have to be processed in order.
It shouldn't take me much to parallelize it. Most fields here are essentially accumulators. Blocks can be read from disk and processed in parallel. Only the dump
...
💬 darosior commented on pull request "validation: detect witness stripping early on":
(https://github.com/bitcoin/bitcoin/pull/33105#issuecomment-3149108432)
Good catch.
(https://github.com/bitcoin/bitcoin/pull/33105#issuecomment-3149108432)
Good catch.
💬 hodlinator commented on pull request "index: store per-block transaction locations for efficient lookups":
(https://github.com/bitcoin/bitcoin/pull/32541#discussion_r2250635349)
Typo in added parenthesis: beggining
(https://github.com/bitcoin/bitcoin/pull/32541#discussion_r2250635349)
Typo in added parenthesis: beggining
💬 hodlinator commented on pull request "index: store per-block transaction locations for efficient lookups":
(https://github.com/bitcoin/bitcoin/pull/32541#discussion_r2250618511)
Better to not add it in 784459ac79a89f591eb52a4c6c266c421ca8baec?
(https://github.com/bitcoin/bitcoin/pull/32541#discussion_r2250618511)
Better to not add it in 784459ac79a89f591eb52a4c6c266c421ca8baec?