💬 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.
💬 theStack commented on pull request "wallet: refactor: various master key encryption cleanups":
(https://github.com/bitcoin/bitcoin/pull/31398#discussion_r1999779103)
The memory consumption of an empty vector is negligible (24 bytes on my system [1]) so this is not a big deal, but happy to address this on the next push.
[1] according to
```
std::vector<unsigned char> empty_vec;
printf("%ld\n", sizeof(empty_vec) + memusage::DynamicUsage(empty_vec));
```
(https://github.com/bitcoin/bitcoin/pull/31398#discussion_r1999779103)
The memory consumption of an empty vector is negligible (24 bytes on my system [1]) so this is not a big deal, but happy to address this on the next push.
[1] according to
```
std::vector<unsigned char> empty_vec;
printf("%ld\n", sizeof(empty_vec) + memusage::DynamicUsage(empty_vec));
```
💬 theStack commented on pull request "wallet: refactor: various master key encryption cleanups":
(https://github.com/bitcoin/bitcoin/pull/31398#discussion_r1999782628)
Good point, ignoring the return values of database seems to be a really bad idea. Should be addressed in a (non-refactor) followup.
(https://github.com/bitcoin/bitcoin/pull/31398#discussion_r1999782628)
Good point, ignoring the return values of database seems to be a really bad idea. Should be addressed in a (non-refactor) followup.
💬 jstefanop commented on issue "LevelDB error: Corruption: block checksum mismatch didn't trigger reindex.":
(https://github.com/bitcoin/bitcoin/issues/30138#issuecomment-2731123565)
> 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.
All non ECC hardware will corrupt eventually, so just a matter of where the line is drawn. We just see it more in real world because of the large number of nodes deployed on the network, and large timeframes for IBD (~ 1 week) (ie 2-4% of all nodes across thousands have a corrup
...
(https://github.com/bitcoin/bitcoin/issues/30138#issuecomment-2731123565)
> 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.
All non ECC hardware will corrupt eventually, so just a matter of where the line is drawn. We just see it more in real world because of the large number of nodes deployed on the network, and large timeframes for IBD (~ 1 week) (ie 2-4% of all nodes across thousands have a corrup
...
💬 mabu44 commented on pull request "wallet: removed duplicate call to GetDescriptorScriptPubKeyMan":
(https://github.com/bitcoin/bitcoin/pull/32023#issuecomment-2731126667)
In some scenarios where the master branch returns a JSON with "success":false, the proposed code throws an error. I think changing RPC behavior without a specific reason should be avoided.
Example:
(in both cases the same descriptor was imported before using a wider [0, 1000] range)
#### On master
```bash
./bitcoin-cli importdescriptors '[{"desc": "wpkh(tprv8ZgxMBicQKsPebj9z5pERWfTpb4RSrqr9tnvGeNPvTV2iaHNdxz3kTcqMmmHabiXpk9DLKE6v24Q6mJwwaLYFLdjtyWs2wtRQUAo3xTGN7i/84h/1h/0h/0/*)#3v9ceyxw"
...
(https://github.com/bitcoin/bitcoin/pull/32023#issuecomment-2731126667)
In some scenarios where the master branch returns a JSON with "success":false, the proposed code throws an error. I think changing RPC behavior without a specific reason should be avoided.
Example:
(in both cases the same descriptor was imported before using a wider [0, 1000] range)
#### On master
```bash
./bitcoin-cli importdescriptors '[{"desc": "wpkh(tprv8ZgxMBicQKsPebj9z5pERWfTpb4RSrqr9tnvGeNPvTV2iaHNdxz3kTcqMmmHabiXpk9DLKE6v24Q6mJwwaLYFLdjtyWs2wtRQUAo3xTGN7i/84h/1h/0h/0/*)#3v9ceyxw"
...
💬 jimhashhq commented on pull request "Multiprocess bitcoin":
(https://github.com/bitcoin/bitcoin/pull/10102#issuecomment-2731137612)
RE multiprocess Bitcoin: Strong concept ACK!
I tried this multiprocess feature out (using cmake/clang/Linux) as follows:
- Full build multiprocess with gui of bitcoin/master (with CMAKE_PREFIX_PATH correctly pointing to a recent external local build of master on libmultiprocess).
- Full build multiprocess with gui of Sjors/02/25/ipc-yea (the multiprocess feature integration branch).
I can confirm that this latter build was indeed simpler with libmultiprocess as a project subtree.
Inte
...
(https://github.com/bitcoin/bitcoin/pull/10102#issuecomment-2731137612)
RE multiprocess Bitcoin: Strong concept ACK!
I tried this multiprocess feature out (using cmake/clang/Linux) as follows:
- Full build multiprocess with gui of bitcoin/master (with CMAKE_PREFIX_PATH correctly pointing to a recent external local build of master on libmultiprocess).
- Full build multiprocess with gui of Sjors/02/25/ipc-yea (the multiprocess feature integration branch).
I can confirm that this latter build was indeed simpler with libmultiprocess as a project subtree.
Inte
...
💬 davidgumberg commented on pull request "net: Block v2->v1 transport downgrade if !fNetworkActive":
(https://github.com/bitcoin/bitcoin/pull/32073#discussion_r1999868812)
Not blocking, but this seemed slightly more correct to me before, since now if `fNetworkActive == false` when caching the result, but `true` when disconnecting, we might not attempt v1 reconnect when we should have.
Enumerating the possible states in the old version where `fNetworkActive` is checked "twice", first to decide whether or not we should flag all `m_nodes` for disconnect, and the "second time" where for each node flagged for disconnect, we check whether or not we should retry v1 be
...
(https://github.com/bitcoin/bitcoin/pull/32073#discussion_r1999868812)
Not blocking, but this seemed slightly more correct to me before, since now if `fNetworkActive == false` when caching the result, but `true` when disconnecting, we might not attempt v1 reconnect when we should have.
Enumerating the possible states in the old version where `fNetworkActive` is checked "twice", first to decide whether or not we should flag all `m_nodes` for disconnect, and the "second time" where for each node flagged for disconnect, we check whether or not we should retry v1 be
...