π fanquake merged a pull request: "clang-tidy: Remove no longer needed NOLINT"
(https://github.com/bitcoin/bitcoin/pull/33781)
(https://github.com/bitcoin/bitcoin/pull/33781)
π¬ fanquake commented on pull request "guix: use GCC 14.3.0 over 13.3.0":
(https://github.com/bitcoin/bitcoin/pull/33775#discussion_r2493801897)
> and the maybe-uninitialized below can be removed?
Fixed the URL, I don't think we can drop it yet: https://github.com/bitcoin/bitcoin/actions/runs/19074211079/job/54485090675?pr=33775#step:8:3723.
(https://github.com/bitcoin/bitcoin/pull/33775#discussion_r2493801897)
> and the maybe-uninitialized below can be removed?
Fixed the URL, I don't think we can drop it yet: https://github.com/bitcoin/bitcoin/actions/runs/19074211079/job/54485090675?pr=33775#step:8:3723.
π¬ hebasto commented on pull request "cmake: Move IPC tests to `ipc/test`":
(https://github.com/bitcoin/bitcoin/pull/33774#issuecomment-3490333396)
> should `src/test/kernel` also be moved to `src/kernel/test` to keep the project structure Consistent. ?
Maybe.
Moving IPC tests provides additional benefits. I'm not sure the same justification applies to the kernel tests, so let's leave them for a follow-up.
(https://github.com/bitcoin/bitcoin/pull/33774#issuecomment-3490333396)
> should `src/test/kernel` also be moved to `src/kernel/test` to keep the project structure Consistent. ?
Maybe.
Moving IPC tests provides additional benefits. I'm not sure the same justification applies to the kernel tests, so let's leave them for a follow-up.
π TheCharlatan approved a pull request: "test: Add bitcoin-chainstate test for assumeutxo functionality"
(https://github.com/bitcoin/bitcoin/pull/33728#pullrequestreview-3421048286)
re-ACK 67740dfe6cfe3f3f91aaad41018a7da9004dcd8c
(https://github.com/bitcoin/bitcoin/pull/33728#pullrequestreview-3421048286)
re-ACK 67740dfe6cfe3f3f91aaad41018a7da9004dcd8c
π willcl-ark approved a pull request: "ci, iwyu: Fix warnings in `src/kernel` and treat them as errors"
(https://github.com/bitcoin/bitcoin/pull/33779#pullrequestreview-3421197426)
ACK a8a33bc0c0a11093418debc36db8ac63bf90e687
Indeed a good time to get this change in :)
(https://github.com/bitcoin/bitcoin/pull/33779#pullrequestreview-3421197426)
ACK a8a33bc0c0a11093418debc36db8ac63bf90e687
Indeed a good time to get this change in :)
π¬ maflcko commented on pull request "guix: use GCC 14.3.0 over 13.3.0":
(https://github.com/bitcoin/bitcoin/pull/33775#issuecomment-3490481643)
lgtm ACK 25ede968badadfeeaaa8358b38c9a8cfff7189f0
(https://github.com/bitcoin/bitcoin/pull/33775#issuecomment-3490481643)
lgtm ACK 25ede968badadfeeaaa8358b38c9a8cfff7189f0
π¬ l0rinc commented on pull request "validation: fetch block inputs on parallel threads >20% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2493940857)
> In this implementation the reserved memory is kept in between blocks
K, so let's reserve outside and clear inside.
Now that missing values aren't failures, we can experiment with shortids - since a missing value isn't a failure anymore (even though I wouldn't expect any collisions in 64 bits either, assuming uniform distribution. But even if the distribution isn't uniform, we can likely store it safely).
64 bit ids for internal spends would mean that in case of some collision we will atte
...
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2493940857)
> In this implementation the reserved memory is kept in between blocks
K, so let's reserve outside and clear inside.
Now that missing values aren't failures, we can experiment with shortids - since a missing value isn't a failure anymore (even though I wouldn't expect any collisions in 64 bits either, assuming uniform distribution. But even if the distribution isn't uniform, we can likely store it safely).
64 bit ids for internal spends would mean that in case of some collision we will atte
...
π fanquake's pull request is ready for review: "guix: use GCC 14.3.0 over 13.3.0"
(https://github.com/bitcoin/bitcoin/pull/33775)
(https://github.com/bitcoin/bitcoin/pull/33775)
π stickies-v approved a pull request: "doc: add cmake help option in Windows build docs"
(https://github.com/bitcoin/bitcoin/pull/33789#pullrequestreview-3421363751)
ACK 9577daa3b88a8fbe8c96d2848adcc88c55c55a29
(https://github.com/bitcoin/bitcoin/pull/33789#pullrequestreview-3421363751)
ACK 9577daa3b88a8fbe8c96d2848adcc88c55c55a29
π rkrux approved a pull request: "rpc: allow writing UTXO set to a named pipe, introduce dump_to_sqlite.sh script"
(https://github.com/bitcoin/bitcoin/pull/31560#pullrequestreview-3421406013)
lgtm ACK ed02f67c5831d94720e55127216aa0d97f55e72a
(https://github.com/bitcoin/bitcoin/pull/31560#pullrequestreview-3421406013)
lgtm ACK ed02f67c5831d94720e55127216aa0d97f55e72a
π¬ rkrux commented on pull request "rpc: allow writing UTXO set to a named pipe, introduce dump_to_sqlite.sh script":
(https://github.com/bitcoin/bitcoin/pull/31560#discussion_r2494075905)
In ed02f67c5831d94720e55127216aa0d97f55e72a "contrib: add script dump_to_sqlite.sh for direct SQLite3 UTXO dump"
Nit: Can call the script `dump_utxo_to_sqlite.sh` for clarity even though it resides in `utxo-tools/`.
(https://github.com/bitcoin/bitcoin/pull/31560#discussion_r2494075905)
In ed02f67c5831d94720e55127216aa0d97f55e72a "contrib: add script dump_to_sqlite.sh for direct SQLite3 UTXO dump"
Nit: Can call the script `dump_utxo_to_sqlite.sh` for clarity even though it resides in `utxo-tools/`.
π hebasto approved a pull request: "doc: add cmake help option in Windows build docs"
(https://github.com/bitcoin/bitcoin/pull/33789#pullrequestreview-3421455626)
ACK 9577daa3b88a8fbe8c96d2848adcc88c55c55a29.
(https://github.com/bitcoin/bitcoin/pull/33789#pullrequestreview-3421455626)
ACK 9577daa3b88a8fbe8c96d2848adcc88c55c55a29.
π¬ naiyoma commented on issue "importdescriptors: check for errors before rescanning":
(https://github.com/bitcoin/bitcoin/issues/33655#issuecomment-3490788538)
Currently, if there are any validation errors, they're added in the result and returned after a rescan . I think we should validate all descriptors first without modifying the wallet. If any descriptor fails validation, throw an error immediately and abortβ no rescan.
A possible fix for this:
```diff
diff --git a/src/wallet/rpc/backup.cpp b/src/wallet/rpc/backup.cpp
index 4b4e43
...
(https://github.com/bitcoin/bitcoin/issues/33655#issuecomment-3490788538)
Currently, if there are any validation errors, they're added in the result and returned after a rescan . I think we should validate all descriptors first without modifying the wallet. If any descriptor fails validation, throw an error immediately and abortβ no rescan.
A possible fix for this:
```diff
diff --git a/src/wallet/rpc/backup.cpp b/src/wallet/rpc/backup.cpp
index 4b4e43
...
π¬ instagibbs commented on pull request "refactor: remove dead branches in `SingletonClusterImpl`":
(https://github.com/bitcoin/bitcoin/pull/33768#issuecomment-3490803624)
ACK 2d23820ee11678d567c75f94c40011ed9f0e274f
(https://github.com/bitcoin/bitcoin/pull/33768#issuecomment-3490803624)
ACK 2d23820ee11678d567c75f94c40011ed9f0e274f
π stickies-v approved a pull request: "script: remove dead code in `CountWitnessSigOps`"
(https://github.com/bitcoin/bitcoin/pull/33786#pullrequestreview-3421542558)
ACK 24bcad3d4df59690f30c9df8ebb62f0bddd0f1c7
(https://github.com/bitcoin/bitcoin/pull/33786#pullrequestreview-3421542558)
ACK 24bcad3d4df59690f30c9df8ebb62f0bddd0f1c7
π Ahfaz001 opened a pull request: "Update README.md"
(https://github.com/bitcoin/bitcoin/pull/33790)
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that improv
...
(https://github.com/bitcoin/bitcoin/pull/33790)
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that improv
...
π¬ l0rinc commented on pull request "log: reduce excessive "rolling back/forward" messages during block replay":
(https://github.com/bitcoin/bitcoin/pull/33443#discussion_r2494434316)
Interesting, though I'm not sure what value hundreds of thousands of logs saying basically the same thing provide.
Do you mind if we tackle this in a follow-up instead?
(https://github.com/bitcoin/bitcoin/pull/33443#discussion_r2494434316)
Interesting, though I'm not sure what value hundreds of thousands of logs saying basically the same thing provide.
Do you mind if we tackle this in a follow-up instead?
π ryanofsky approved a pull request: "cmake: Move IPC tests to `ipc/test`"
(https://github.com/bitcoin/bitcoin/pull/33774#pullrequestreview-3421970095)
Code review ACK f26c8b03f968843e6e1381d1e1721f4379f7e7ab. Nice change avoiding the need to duplicate clang-tidy suppressions. Thanks for the followup!
(https://github.com/bitcoin/bitcoin/pull/33774#pullrequestreview-3421970095)
Code review ACK f26c8b03f968843e6e1381d1e1721f4379f7e7ab. Nice change avoiding the need to duplicate clang-tidy suppressions. Thanks for the followup!
π¬ ryanofsky commented on pull request "cmake: Move IPC tests to `ipc/test`":
(https://github.com/bitcoin/bitcoin/pull/33774#discussion_r2494445665)
In commit "cmake, test: Improve locality of `bitcoin_ipc_test` library description" (f26c8b03f968843e6e1381d1e1721f4379f7e7ab)
I think this comment is still useful to keep because otherwise there is no explanation of why `bitcoin_ipc_test` in defined `src/ipc/` instead of `src/ipc/test` where it would make more sense.
Comment would just need to be tweaked slightly to be kept: `src/CMakeLists.txt` -> `src/ipc/CMakeLists.txt`, `src/test/CMakeLists.txt` -> `src/ipc/test/CMakeLists.txt`, and `
...
(https://github.com/bitcoin/bitcoin/pull/33774#discussion_r2494445665)
In commit "cmake, test: Improve locality of `bitcoin_ipc_test` library description" (f26c8b03f968843e6e1381d1e1721f4379f7e7ab)
I think this comment is still useful to keep because otherwise there is no explanation of why `bitcoin_ipc_test` in defined `src/ipc/` instead of `src/ipc/test` where it would make more sense.
Comment would just need to be tweaked slightly to be kept: `src/CMakeLists.txt` -> `src/ipc/CMakeLists.txt`, `src/test/CMakeLists.txt` -> `src/ipc/test/CMakeLists.txt`, and `
...
π¬ yancyribbens commented on issue "dnsseed.bitcoin.dashjr-list-of-p2p-nodes.us appears to be violating DNS seed policy":
(https://github.com/bitcoin/bitcoin/issues/33734#issuecomment-3491224946)
Forgive my interjection, although isn't there practice in place to routinely audit DNS seed needs? IE a chron job that checks what versions are being named and the validity of the content? If such an audit system is in place, run on publicly verifiable infrastructure, then there's indisputable reason to remove a node that names invalid content. Also, if there is a high bar for auditing seed DNS seed nodes, I see no reason why more people can't run these nodes, not less.
Also, it would be rea
...
(https://github.com/bitcoin/bitcoin/issues/33734#issuecomment-3491224946)
Forgive my interjection, although isn't there practice in place to routinely audit DNS seed needs? IE a chron job that checks what versions are being named and the validity of the content? If such an audit system is in place, run on publicly verifiable infrastructure, then there's indisputable reason to remove a node that names invalid content. Also, if there is a high bar for auditing seed DNS seed nodes, I see no reason why more people can't run these nodes, not less.
Also, it would be rea
...