👍 pinheadmz approved a pull request: "rpc: Round verificationprogress to 1 for a recent tip"
(https://github.com/bitcoin/bitcoin/pull/32528#pullrequestreview-2871206816)
ACK fab1e02086ceebd7d96417b7a386fe61158bfda9
Trivial code cleanup changes since last ack. Also re-built and tested on macos/arm64
<details><summary>Show Signature</summary>
```
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256
ACK fab1e02086ceebd7d96417b7a386fe61158bfda9
-----BEGIN PGP SIGNATURE-----
iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmg1wvoACgkQ5+KYS2KJ
yTrrtBAAhzspf6yfD6do0zH7Cu0I4OMA1C/KlabXxcyA9O4Foa/sNjx//iXEKFIX
EO0fEfcGZ+cWwlpMGG5TD+NfPl6jKfVb376xxESSUT4HvUIOB/
...
(https://github.com/bitcoin/bitcoin/pull/32528#pullrequestreview-2871206816)
ACK fab1e02086ceebd7d96417b7a386fe61158bfda9
Trivial code cleanup changes since last ack. Also re-built and tested on macos/arm64
<details><summary>Show Signature</summary>
```
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256
ACK fab1e02086ceebd7d96417b7a386fe61158bfda9
-----BEGIN PGP SIGNATURE-----
iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmg1wvoACgkQ5+KYS2KJ
yTrrtBAAhzspf6yfD6do0zH7Cu0I4OMA1C/KlabXxcyA9O4Foa/sNjx//iXEKFIX
EO0fEfcGZ+cWwlpMGG5TD+NfPl6jKfVb376xxESSUT4HvUIOB/
...
💬 theStack commented on pull request "contrib: utxo_to_sqlite.py: add option to store txid/spk as BLOBs":
(https://github.com/bitcoin/bitcoin/pull/32621#issuecomment-2912629691)
@ajtowns: Thanks for your suggestions, I like the idea of having more flexibility and mostly agree with the naming. Took your patch (TIL about [argparse `choices`](https://docs.python.org/3/library/argparse.html#choices)) and adapted the functional test accordingly, to exercise all combinations of new options. As for txid format option naming, I think I'd intuitively prefer `raw` for storing them in the hash order, since that's AFAICT the most common txid byte-string serialization (I think I hav
...
(https://github.com/bitcoin/bitcoin/pull/32621#issuecomment-2912629691)
@ajtowns: Thanks for your suggestions, I like the idea of having more flexibility and mostly agree with the naming. Took your patch (TIL about [argparse `choices`](https://docs.python.org/3/library/argparse.html#choices)) and adapted the functional test accordingly, to exercise all combinations of new options. As for txid format option naming, I think I'd intuitively prefer `raw` for storing them in the hash order, since that's AFAICT the most common txid byte-string serialization (I think I hav
...
💬 Sjors commented on pull request "Add checkBlock() to Mining interface":
(https://github.com/bitcoin/bitcoin/pull/31981#discussion_r2109284630)
This doesn't compile, but I assume you also meant to pass `view_dummy` into the `ConnectBlock` call below? I don't remember what the original was based on, so took the suggestion.
(https://github.com/bitcoin/bitcoin/pull/31981#discussion_r2109284630)
This doesn't compile, but I assume you also meant to pass `view_dummy` into the `ConnectBlock` call below? I don't remember what the original was based on, so took the suggestion.
💬 hebasto commented on pull request "ci, iwyu: Treat warnings as errors for specific targets":
(https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2109307871)
1. The [change](https://github.com/bitcoin/bitcoin/compare/16c69e927cccc1e93beaa10c621afd6a8f422e6c..0c3c7d07599d0086d5151949a69ffc12b6166d19) touches 8 files. The modification in `src/crypto/siphash.h` is a no-op from IWYU's perspective.
2. CMake integrates the IWYU into the compilation step. Therefore, it reports an error for each object file individually.
However, the diagnostic output includes both headers and the source file:
```
[12:32:50.757] Warning: include-what-you-use reported d
...
(https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2109307871)
1. The [change](https://github.com/bitcoin/bitcoin/compare/16c69e927cccc1e93beaa10c621afd6a8f422e6c..0c3c7d07599d0086d5151949a69ffc12b6166d19) touches 8 files. The modification in `src/crypto/siphash.h` is a no-op from IWYU's perspective.
2. CMake integrates the IWYU into the compilation step. Therefore, it reports an error for each object file individually.
However, the diagnostic output includes both headers and the source file:
```
[12:32:50.757] Warning: include-what-you-use reported d
...
💬 pinheadmz commented on pull request "blocks: avoid recomputing block header hash in `ReadBlock`":
(https://github.com/bitcoin/bitcoin/pull/32487#issuecomment-2912704473)
Is there a reason we don't just cache the hash as a member of `CBlockHeader` and compute-or-return that value in `GetHash()`?
(https://github.com/bitcoin/bitcoin/pull/32487#issuecomment-2912704473)
Is there a reason we don't just cache the hash as a member of `CBlockHeader` and compute-or-return that value in `GetHash()`?
🤔 instagibbs reviewed a pull request: "cluster mempool: add TxGraph reorg functionality"
(https://github.com/bitcoin/bitcoin/pull/31553#pullrequestreview-2854383890)
Reviewed through 538e9ff804f8dfba6f6a808e83572fdeab145ab8
Last big Q is about the cycles being generated in the implied graph, and what if anything we should do differently about it.
(https://github.com/bitcoin/bitcoin/pull/31553#pullrequestreview-2854383890)
Reviewed through 538e9ff804f8dfba6f6a808e83572fdeab145ab8
Last big Q is about the cycles being generated in the implied graph, and what if anything we should do differently about it.
💬 instagibbs commented on pull request "cluster mempool: add TxGraph reorg functionality":
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r2098113781)
```Suggestion
const LinearizationChunking linchunking(m_depgraph, m_linearization);
```
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r2098113781)
```Suggestion
const LinearizationChunking linchunking(m_depgraph, m_linearization);
```
💬 instagibbs commented on pull request "cluster mempool: add TxGraph reorg functionality":
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r2098239806)
nit: leave a comment that deps is also being added to here, briefly forgot
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r2098239806)
nit: leave a comment that deps is also being added to here, briefly forgot
💬 instagibbs commented on pull request "cluster mempool: add TxGraph reorg functionality":
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r2098190802)
nit: 3 out of the N fields are being set here, could we be specific which ones for clarity?
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r2098190802)
nit: 3 out of the N fields are being set here, could we be specific which ones for clarity?
💬 instagibbs commented on pull request "cluster mempool: add TxGraph reorg functionality":
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r2098240846)
```Suggestion
// Append explicit deps, and sort it by child.
```
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r2098240846)
```Suggestion
// Append explicit deps, and sort it by child.
```
💬 instagibbs commented on pull request "cluster mempool: add TxGraph reorg functionality":
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r2098254652)
```Suggestion
// Sort deps by parent by graph index. The order is unchanged from now on.
```
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r2098254652)
```Suggestion
// Sort deps by parent by graph index. The order is unchanged from now on.
```
💬 instagibbs commented on pull request "cluster mempool: add TxGraph reorg functionality":
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r2098251956)
```Suggestion
// Fill m_deps_left in trim_data and initially populate trim_heap. Because of the sort above, all
```
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r2098251956)
```Suggestion
// Fill m_deps_left in trim_data and initially populate trim_heap. Because of the sort above, all
```
💬 instagibbs commented on pull request "cluster mempool: add TxGraph reorg functionality":
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r2098265434)
```Suggestion
// a map from GraphIndex to TrimTxData via locate_fn, and its ordering will not change anymore.
```
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r2098265434)
```Suggestion
// a map from GraphIndex to TrimTxData via locate_fn, and its ordering will not change anymore.
```
💬 instagibbs commented on pull request "cluster mempool: add TxGraph reorg functionality":
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r2098308679)
```Suggestion
Assume(!ret.empty());
return ret;
```
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r2098308679)
```Suggestion
Assume(!ret.empty());
return ret;
```
💬 instagibbs commented on pull request "cluster mempool: add TxGraph reorg functionality":
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r2109385109)
https://github.com/instagibbs/bitcoin/tree/2025-05-applytrim
Worked on a test attempts to cover the claim here. It ended up being more complicated than that (claim a bit wrong?) due to the lazy application of dependencies. For this property to hold, I believe you have to touch the TxGraph in any way that applies dependencies when undersized, *then* transition to oversized graph for the circular dependencies to not occur in the implied graph.
Without the dependencies applied, the linearizat
...
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r2109385109)
https://github.com/instagibbs/bitcoin/tree/2025-05-applytrim
Worked on a test attempts to cover the claim here. It ended up being more complicated than that (claim a bit wrong?) due to the lazy application of dependencies. For this property to hold, I believe you have to touch the TxGraph in any way that applies dependencies when undersized, *then* transition to oversized graph for the circular dependencies to not occur in the implied graph.
Without the dependencies applied, the linearizat
...
🤔 sipa reviewed a pull request: "rpc: Round verificationprogress to 1 for a recent tip"
(https://github.com/bitcoin/bitcoin/pull/32528#pullrequestreview-2871433824)
utACK fab1e02086ceebd7d96417b7a386fe61158bfda9
(https://github.com/bitcoin/bitcoin/pull/32528#pullrequestreview-2871433824)
utACK fab1e02086ceebd7d96417b7a386fe61158bfda9
💬 luke-jr commented on pull request "subprocess: Let shell parse command on non-Windows systems":
(https://github.com/bitcoin/bitcoin/pull/32577#issuecomment-2912964132)
Probably should really go through the shell on Windows too...
(https://github.com/bitcoin/bitcoin/pull/32577#issuecomment-2912964132)
Probably should really go through the shell on Windows too...
💬 Kixunil commented on pull request "contrib: add xor-blocks tool to obfuscate blocks directory":
(https://github.com/bitcoin/bitcoin/pull/32451#discussion_r2109501741)
Yes, but it loses the error information so you'd silently continue even if write failed leading to data corruption.
(https://github.com/bitcoin/bitcoin/pull/32451#discussion_r2109501741)
Yes, but it loses the error information so you'd silently continue even if write failed leading to data corruption.
💬 Kixunil commented on pull request "contrib: add xor-blocks tool to obfuscate blocks directory":
(https://github.com/bitcoin/bitcoin/pull/32451#discussion_r2109507090)
On the next run it'll crash failing to convert into `[u8; 8]`. While not catastrophic, it makes the program more annoying for end users.
(https://github.com/bitcoin/bitcoin/pull/32451#discussion_r2109507090)
On the next run it'll crash failing to convert into `[u8; 8]`. While not catastrophic, it makes the program more annoying for end users.
🤔 l0rinc requested changes to a pull request: "improve MallocUsage() accuracy"
(https://github.com/bitcoin/bitcoin/pull/28531#pullrequestreview-2871139248)
Thanks for getting back to this change.
We should synchronize it with `CCoinsMap`, chich already claims to account for an "overhead of 1 or 2 pointers".
Also, the test should pass before the change as well, so it would be helpful to add it as a separate commit before the change to make it easy to see how it behaves before and after the change as well.
The test still contains a lot of repetition, please see my suggestion on how to compact it a bit more.
There are still uncovered parts of the
...
(https://github.com/bitcoin/bitcoin/pull/28531#pullrequestreview-2871139248)
Thanks for getting back to this change.
We should synchronize it with `CCoinsMap`, chich already claims to account for an "overhead of 1 or 2 pointers".
Also, the test should pass before the change as well, so it would be helpful to add it as a separate commit before the change to make it easy to see how it behaves before and after the change as well.
The test still contains a lot of repetition, please see my suggestion on how to compact it a bit more.
There are still uncovered parts of the
...