⚠️ maflcko reopened an issue: "intermittent issue in p2p_orphan_handling.py"
(https://github.com/bitcoin/bitcoin/issues/31700)
Looks like the test may overall intermittently fail, because it assumes the tx delay isn't exponentially distributed, but has a constant delay, sufficient for the test to pass.
I think it would be better to use mocktime to halt the test when needed, so that it can even pass when the delay is zero (or close to it).
I haven't looked in detail whether this is the same, but there was one instance in the CI: (Possibly the getdata was split into two, so the check didn't pick it up, because it assume
...
(https://github.com/bitcoin/bitcoin/issues/31700)
Looks like the test may overall intermittently fail, because it assumes the tx delay isn't exponentially distributed, but has a constant delay, sufficient for the test to pass.
I think it would be better to use mocktime to halt the test when needed, so that it can even pass when the delay is zero (or close to it).
I haven't looked in detail whether this is the same, but there was one instance in the CI: (Possibly the getdata was split into two, so the check didn't pick it up, because it assume
...
💬 maflcko commented on pull request "qt: Add addressList field to SendCoinsRecipient for multiple addresses":
(https://github.com/bitcoin-core/gui/pull/857#issuecomment-2730699636)
You'll also need to make sure the test, and CI pass. Also, there needs to be a test for a new feature.
(https://github.com/bitcoin-core/gui/pull/857#issuecomment-2730699636)
You'll also need to make sure the test, and CI pass. Also, there needs to be a test for a new feature.
💬 maflcko commented on pull request "fuzz: Speed up *_package_eval fuzz targets a bit":
(https://github.com/bitcoin/bitcoin/pull/31457#discussion_r1999553734)
`mempool_outpoints` can't be changed, because the order matters here. An unordered_set isn't ordered and may result in a non-deterministic fuzz target, given the same fuzz input.
(https://github.com/bitcoin/bitcoin/pull/31457#discussion_r1999553734)
`mempool_outpoints` can't be changed, because the order matters here. An unordered_set isn't ordered and may result in a non-deterministic fuzz target, given the same fuzz input.
💬 l0rinc commented on pull request "fuzz: Speed up *_package_eval fuzz targets a bit":
(https://github.com/bitcoin/bitcoin/pull/31457#discussion_r1999563234)
Ah, that's what https://github.com/bitcoin/bitcoin/pull/31457#discussion_r1967477525 meant - please resolve this comment
(https://github.com/bitcoin/bitcoin/pull/31457#discussion_r1999563234)
Ah, that's what https://github.com/bitcoin/bitcoin/pull/31457#discussion_r1967477525 meant - please resolve this comment
💬 maflcko commented on pull request "fuzz: Speed up *_package_eval fuzz targets a bit":
(https://github.com/bitcoin/bitcoin/pull/31457#discussion_r1999589480)
Not sure if the two are comparable, inserting into an empty set or inserting into a set with one value (probably the most common cases) seems plausible to be faster than doing the same in an unordered_set. The goal here is to avoid fuzz timeouts (or long running large fuzz inputs) when using sanitizers.
(https://github.com/bitcoin/bitcoin/pull/31457#discussion_r1999589480)
Not sure if the two are comparable, inserting into an empty set or inserting into a set with one value (probably the most common cases) seems plausible to be faster than doing the same in an unordered_set. The goal here is to avoid fuzz timeouts (or long running large fuzz inputs) when using sanitizers.
💬 maflcko commented on pull request "contrib: Add `deterministic-fuzz-coverage/Cargo.lock` to version control":
(https://github.com/bitcoin/bitcoin/pull/32082#issuecomment-2730858179)
Duplicate of fa3940b1cbc94c8ccfde36be1db1adca04fbcaa6?
Otherwise, needs rebase.
(https://github.com/bitcoin/bitcoin/pull/32082#issuecomment-2730858179)
Duplicate of fa3940b1cbc94c8ccfde36be1db1adca04fbcaa6?
Otherwise, needs rebase.
💬 maflcko commented on pull request "contrib: Add deterministic-fuzz-coverage":
(https://github.com/bitcoin/bitcoin/pull/31836#issuecomment-2730873997)
> After this change I keep getting `untracked files will be overwritten by rebase` for `contrib/devtools/deterministic-fuzz-coverage/Cargo.lock`: added a follow-up to commit it #32082
When testing locally, my recommendation would be to generally try to rebase a pull request onto current master. This ensures you get the latest fixes, keep your ccache temperature high by focussing on master, and may even find silent merge conflicts.
(https://github.com/bitcoin/bitcoin/pull/31836#issuecomment-2730873997)
> After this change I keep getting `untracked files will be overwritten by rebase` for `contrib/devtools/deterministic-fuzz-coverage/Cargo.lock`: added a follow-up to commit it #32082
When testing locally, my recommendation would be to generally try to rebase a pull request onto current master. This ensures you get the latest fixes, keep your ccache temperature high by focussing on master, and may even find silent merge conflicts.
💬 maflcko commented on pull request "doc: shallow clone `qa-assets`":
(https://github.com/bitcoin/bitcoin/pull/32083#issuecomment-2730877899)
lgtm ACK 6f9f415a4fa3a3c1dc87c207b0192a5ed0a91e0f
(https://github.com/bitcoin/bitcoin/pull/32083#issuecomment-2730877899)
lgtm ACK 6f9f415a4fa3a3c1dc87c207b0192a5ed0a91e0f
✅ l0rinc closed a pull request: "contrib: Add `deterministic-fuzz-coverage/Cargo.lock` to version control"
(https://github.com/bitcoin/bitcoin/pull/32082)
(https://github.com/bitcoin/bitcoin/pull/32082)
💬 l0rinc commented on pull request "contrib: Add `deterministic-fuzz-coverage/Cargo.lock` to version control":
(https://github.com/bitcoin/bitcoin/pull/32082#issuecomment-2730898492)
Seems I made a stupid mistake and didn't rebase this one before pushing - my bad.
(https://github.com/bitcoin/bitcoin/pull/32082#issuecomment-2730898492)
Seems I made a stupid mistake and didn't rebase this one before pushing - my bad.
💬 mzumsande commented on pull request "test: fix intermittent failure in p2p_orphan_handling.py":
(https://github.com/bitcoin/bitcoin/pull/32063#issuecomment-2730898960)
Interesting - wouldn't that mean that `disconnect_p2ps` which is used in a lot of places has the same issue?
(https://github.com/bitcoin/bitcoin/pull/32063#issuecomment-2730898960)
Interesting - wouldn't that mean that `disconnect_p2ps` which is used in a lot of places has the same issue?
💬 hodlinator commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1999690309)
Had a feeling `OSError`s must always have that field set (except for the socket `TimeoutError` bug), but the [docs](https://docs.python.org/3/library/exceptions.html#OSError) indicate it's a bit more loose:
> The second form of the constructor sets the corresponding attributes, described below. The attributes default to None if not specified.
Could change the condition as you suggestion, but it would be prudent to assert that `error_num` isn't `None` for other cases:
```python
...
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1999690309)
Had a feeling `OSError`s must always have that field set (except for the socket `TimeoutError` bug), but the [docs](https://docs.python.org/3/library/exceptions.html#OSError) indicate it's a bit more loose:
> The second form of the constructor sets the corresponding attributes, described below. The attributes default to None if not specified.
Could change the condition as you suggestion, but it would be prudent to assert that `error_num` isn't `None` for other cases:
```python
...
💬 l0rinc commented on pull request "fuzz: Speed up *_package_eval fuzz targets a bit":
(https://github.com/bitcoin/bitcoin/pull/31457#discussion_r1999692015)
Could you please add this conclusion to the PR description, I misinterpreted "the element order is not used in the fuzz test" as meaning the previous discussions can be discarded :/
(https://github.com/bitcoin/bitcoin/pull/31457#discussion_r1999692015)
Could you please add this conclusion to the PR description, I misinterpreted "the element order is not used in the fuzz test" as meaning the previous discussions can be discarded :/
💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-2731007985)
Updated 5991a69ee0000de551955846d7d21733c326a748 -> 2dc27e2860b97c2bffa5f18706917b21858e5594 ([kernelApi_29](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_29) -> [kernelApi_30](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_30), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelApi_29..kernelApi_30))
* Addressed @stickies-v's [comment](https://github.com/bitcoin/bitcoin/pull/30595#discussion_r1999116127), add notice in the documentation about logging settings be
...
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-2731007985)
Updated 5991a69ee0000de551955846d7d21733c326a748 -> 2dc27e2860b97c2bffa5f18706917b21858e5594 ([kernelApi_29](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_29) -> [kernelApi_30](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_30), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelApi_29..kernelApi_30))
* Addressed @stickies-v's [comment](https://github.com/bitcoin/bitcoin/pull/30595#discussion_r1999116127), add notice in the documentation about logging settings be
...
📝 mzumsande converted_to_draft a pull request: "validation: stricter internal handling of invalid blocks"
(https://github.com/bitcoin/bitcoin/pull/31405)
Some fields in validation are set opportunistically by "best effort":
- The `BLOCK_FAILED_CHILD` status (which means that the block index has an invalid predecessor)
- `m_best_header` (the most-work header not known to be invalid).
This means that there are known situations in which these fields are not set when they should be, or set to wrong values. This is tolerated because the fields are not used for anything consensus-critical and triggering these situations involved creating invalid b
...
(https://github.com/bitcoin/bitcoin/pull/31405)
Some fields in validation are set opportunistically by "best effort":
- The `BLOCK_FAILED_CHILD` status (which means that the block index has an invalid predecessor)
- `m_best_header` (the most-work header not known to be invalid).
This means that there are known situations in which these fields are not set when they should be, or set to wrong values. This is tolerated because the fields are not used for anything consensus-critical and triggering these situations involved creating invalid b
...
💬 l0rinc commented on pull request "fuzz: Speed up *_package_eval fuzz targets a bit":
(https://github.com/bitcoin/bitcoin/pull/31457#issuecomment-2731061683)
I rebased and ran the `tx_package_eval` fuzz corpora with a fixed seed before and after this change, 2 times per commit.
It indicates a 13% speedup - the suggested additional reserve calls don't seem to matter at all.
<details>
<summary>Reproducer benchmark</summary>
```bash
git clone --depth=1 https://github.com/bitcoin-core/qa-assets 2>/dev/null || true; \
COMMITS="86f13e7ab17a9a38172398e09b92bc63a10cc60b 6323f196ca7e69e1052a4c7e7a5beeab0010f8fa fd161bd9fdeced1ee929d86946ee205ea6cc72
...
(https://github.com/bitcoin/bitcoin/pull/31457#issuecomment-2731061683)
I rebased and ran the `tx_package_eval` fuzz corpora with a fixed seed before and after this change, 2 times per commit.
It indicates a 13% speedup - the suggested additional reserve calls don't seem to matter at all.
<details>
<summary>Reproducer benchmark</summary>
```bash
git clone --depth=1 https://github.com/bitcoin-core/qa-assets 2>/dev/null || true; \
COMMITS="86f13e7ab17a9a38172398e09b92bc63a10cc60b 6323f196ca7e69e1052a4c7e7a5beeab0010f8fa fd161bd9fdeced1ee929d86946ee205ea6cc72
...
💬 jstefanop commented on issue "LevelDB error: Corruption: block checksum mismatch didn't trigger reindex.":
(https://github.com/bitcoin/bitcoin/issues/30138#issuecomment-2731062047)
> > This should really be looked into, seeing this a lot more with our hardware now that blocks are full
>
> If the chainstate is getting corrupt, that points to a hardware problem, and that is where it should be fixed.
>
> > re-indexing entire blockchain for a corrupt chainstate a few hundred blocks from tip is horrible UX.
>
> The issue here is that it _isn't_ reindexing.
>
> > Should only require reindex from last good block db.
>
> Bitcoin Core does not store old block dbs. It has a sin
...
(https://github.com/bitcoin/bitcoin/issues/30138#issuecomment-2731062047)
> > This should really be looked into, seeing this a lot more with our hardware now that blocks are full
>
> If the chainstate is getting corrupt, that points to a hardware problem, and that is where it should be fixed.
>
> > re-indexing entire blockchain for a corrupt chainstate a few hundred blocks from tip is horrible UX.
>
> The issue here is that it _isn't_ reindexing.
>
> > Should only require reindex from last good block db.
>
> Bitcoin Core does not store old block dbs. It has a sin
...
💬 sipa commented on issue "LevelDB error: Corruption: block checksum mismatch didn't trigger reindex.":
(https://github.com/bitcoin/bitcoin/issues/30138#issuecomment-2731075013)
If your hardware regularly corrupts the database, the system is just too unreliable, and there is little that can be done long term. Even with a regular snapshot feature, it might even corrupt while taking a snapshot.
(https://github.com/bitcoin/bitcoin/issues/30138#issuecomment-2731075013)
If your hardware regularly corrupts the database, the system is just too unreliable, and there is little that can be done long term. Even with a regular snapshot feature, it might even corrupt while taking a snapshot.
💬 tnndbtc commented on pull request "test: fix intermittent failure in p2p_orphan_handling.py":
(https://github.com/bitcoin/bitcoin/pull/32063#issuecomment-2731097839)
@mzumsande I have another finding, if I move the calling of `self.wait_until(lambda: len(node.getorphantxs(verbosity=2)[0]["from"]) == 1)` before `node.bumpmocktime(TXREQUEST_TIME_SKIP)`, then this issue is not reproducible even I set a longer sleep before calling `DeleteNode(pnode);`:
```
# Disconnect peer1. peer2 should become the new candidate for orphan resolution.
peer1.peer_disconnect()
self.wait_until(lambda: len(node.getorphantxs(verbosity=2)[0]["from"]) ==
...
(https://github.com/bitcoin/bitcoin/pull/32063#issuecomment-2731097839)
@mzumsande I have another finding, if I move the calling of `self.wait_until(lambda: len(node.getorphantxs(verbosity=2)[0]["from"]) == 1)` before `node.bumpmocktime(TXREQUEST_TIME_SKIP)`, then this issue is not reproducible even I set a longer sleep before calling `DeleteNode(pnode);`:
```
# Disconnect peer1. peer2 should become the new candidate for orphan resolution.
peer1.peer_disconnect()
self.wait_until(lambda: len(node.getorphantxs(verbosity=2)[0]["from"]) ==
...
💬 theStack commented on pull request "wallet: refactor: various master key encryption cleanups":
(https://github.com/bitcoin/bitcoin/pull/31398#issuecomment-2731112457)
Thanks for the reviews! The suggestions (https://github.com/bitcoin/bitcoin/pull/31398#discussion_r1931016480, https://github.com/bitcoin/bitcoin/pull/31398#discussion_r1994315843, https://github.com/bitcoin/bitcoin/pull/31398#discussion_r1996031367 and https://github.com/bitcoin/bitcoin/pull/31398#discussion_r1996031577) make sense, but since they are all nits (IMHO), I'll address them if I have to retouch.
(https://github.com/bitcoin/bitcoin/pull/31398#issuecomment-2731112457)
Thanks for the reviews! The suggestions (https://github.com/bitcoin/bitcoin/pull/31398#discussion_r1931016480, https://github.com/bitcoin/bitcoin/pull/31398#discussion_r1994315843, https://github.com/bitcoin/bitcoin/pull/31398#discussion_r1996031367 and https://github.com/bitcoin/bitcoin/pull/31398#discussion_r1996031577) make sense, but since they are all nits (IMHO), I'll address them if I have to retouch.