🚀 fanquake merged a pull request: "test: fix and augment block tests of invalid_txs"
(https://github.com/bitcoin/bitcoin/pull/32591)
(https://github.com/bitcoin/bitcoin/pull/32591)
🤔 rkrux reviewed a pull request: "wallet, rpc: Return normalized descriptor in parent_descs"
(https://github.com/bitcoin/bitcoin/pull/32594#pullrequestreview-2870949997)
ACK a759a22d59e805834d077a28c95695e4834983a9
Agree that normalized descriptors are more useful as no further hardened derivation is required.
Not suggesting any change, only a question: Specific for the `listunspent` RPC though where both `parent_descs` and `desc` fields are present, is `parent_descs` field useful only when the unspent is not solvable? Because `parent_descs` seem like the superset of `desc` from few values I checked and also by its name.
(https://github.com/bitcoin/bitcoin/pull/32594#pullrequestreview-2870949997)
ACK a759a22d59e805834d077a28c95695e4834983a9
Agree that normalized descriptors are more useful as no further hardened derivation is required.
Not suggesting any change, only a question: Specific for the `listunspent` RPC though where both `parent_descs` and `desc` fields are present, is `parent_descs` field useful only when the unspent is not solvable? Because `parent_descs` seem like the superset of `desc` from few values I checked and also by its name.
💬 rkrux commented on pull request "wallet, rpc: Return normalized descriptor in parent_descs":
(https://github.com/bitcoin/bitcoin/pull/32594#discussion_r2109239705)
IIUC, this is just a dummy signing provider being passed here and essentially the last hardened xpub would be retrieved from the desc cache because I don't see this provider being populated in the call stack.
https://github.com/bitcoin/bitcoin/blob/f7cc7f6468afeb20b01ee86575c7b6329ed2faf9/src/script/descriptor.cpp#L533-L536
Ideally, passing a null for the provider arg could have been better to emphasise on this but I see the function signature accepts a reference instead.
https://github
...
(https://github.com/bitcoin/bitcoin/pull/32594#discussion_r2109239705)
IIUC, this is just a dummy signing provider being passed here and essentially the last hardened xpub would be retrieved from the desc cache because I don't see this provider being populated in the call stack.
https://github.com/bitcoin/bitcoin/blob/f7cc7f6468afeb20b01ee86575c7b6329ed2faf9/src/script/descriptor.cpp#L533-L536
Ideally, passing a null for the provider arg could have been better to emphasise on this but I see the function signature accepts a reference instead.
https://github
...
💬 rkrux commented on pull request "wallet, rpc: Return normalized descriptor in parent_descs":
(https://github.com/bitcoin/bitcoin/pull/32594#discussion_r2109107403)
Nit: Most likely this sentence was reworded midway because it ends abruptly on first line and the content is duplicated on the next.
(https://github.com/bitcoin/bitcoin/pull/32594#discussion_r2109107403)
Nit: Most likely this sentence was reworded midway because it ends abruptly on first line and the content is duplicated on the next.
👍 hebasto approved a pull request: "test: Add missing ipc subtree to lint"
(https://github.com/bitcoin/bitcoin/pull/32623#pullrequestreview-2871162934)
ACK fa2bf6bdb7e5f8940fe4bce1aaab29bf2e063b49.
(https://github.com/bitcoin/bitcoin/pull/32623#pullrequestreview-2871162934)
ACK fa2bf6bdb7e5f8940fe4bce1aaab29bf2e063b49.
💬 hebasto commented on pull request "test: Add missing ipc subtree to lint":
(https://github.com/bitcoin/bitcoin/pull/32623#discussion_r2109243775)
Maybe add a reverse reference to this list in `test/lint/test_runner/src/main.rs`?
(https://github.com/bitcoin/bitcoin/pull/32623#discussion_r2109243775)
Maybe add a reverse reference to this list in `test/lint/test_runner/src/main.rs`?
💬 andrewtoth commented on pull request "contrib: add xor-blocks tool to obfuscate blocks directory":
(https://github.com/bitcoin/bitcoin/pull/32451#discussion_r2109251771)
Does it matter though? If it crashes before sync_all we either have all zero in xor.dat or a random key that has not yet been used for xoring anything. On next run it will either rewrite a new key if still zero or start using the previously written random key.
(https://github.com/bitcoin/bitcoin/pull/32451#discussion_r2109251771)
Does it matter though? If it crashes before sync_all we either have all zero in xor.dat or a random key that has not yet been used for xoring anything. On next run it will either rewrite a new key if still zero or start using the previously written random key.
💬 andrewtoth commented on pull request "contrib: add xor-blocks tool to obfuscate blocks directory":
(https://github.com/bitcoin/bitcoin/pull/32451#discussion_r2109256380)
`into_inner` flushes before returning. https://doc.rust-lang.org/std/io/struct.BufWriter.html#method.into_inner
(https://github.com/bitcoin/bitcoin/pull/32451#discussion_r2109256380)
`into_inner` flushes before returning. https://doc.rust-lang.org/std/io/struct.BufWriter.html#method.into_inner
🤔 jonatack reviewed a pull request: "blocks: avoid recomputing block header hash in `ReadBlock`"
(https://github.com/bitcoin/bitcoin/pull/32487#pullrequestreview-2871187569)
ACK 09ee8b7f278627b917f0784adf23cbc76cae5fa0
(https://github.com/bitcoin/bitcoin/pull/32487#pullrequestreview-2871187569)
ACK 09ee8b7f278627b917f0784adf23cbc76cae5fa0
💬 jonatack commented on pull request "blocks: avoid recomputing block header hash in `ReadBlock`":
(https://github.com/bitcoin/bitcoin/pull/32487#discussion_r2109259969)
If you retouch now or in another pull, would be clearer to state which blockhash is expected vs actual.
```suggestion
LogError("GetHash() doesn't match index at %s while reading block (actual %s != expected %s)",
```
(https://github.com/bitcoin/bitcoin/pull/32487#discussion_r2109259969)
If you retouch now or in another pull, would be clearer to state which blockhash is expected vs actual.
```suggestion
LogError("GetHash() doesn't match index at %s while reading block (actual %s != expected %s)",
```
✅ andrewtoth closed a pull request: "contrib: add xor-blocks tool to obfuscate blocks directory"
(https://github.com/bitcoin/bitcoin/pull/32451)
(https://github.com/bitcoin/bitcoin/pull/32451)
👍 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?