💬 hodlinator commented on pull request "headerssync: Keep tests ahead of increasing params":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2115848183)
I like the idea of including the generation date, and was including changes to the Python generator file in an unpublished version of this PR, but decided to keep that separate. Added to that separate WIP branch.
The magic numbers are not updated manually, they are included in the calculated output:
https://github.com/bitcoin/bitcoin/blob/11a2d3a63e90cdc1920ede3c67d52a9c72860e6b/contrib/devtools/headerssync-params.py#L347-L348
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2115848183)
I like the idea of including the generation date, and was including changes to the Python generator file in an unpublished version of this PR, but decided to keep that separate. Added to that separate WIP branch.
The magic numbers are not updated manually, they are included in the calculated output:
https://github.com/bitcoin/bitcoin/blob/11a2d3a63e90cdc1920ede3c67d52a9c72860e6b/contrib/devtools/headerssync-params.py#L347-L348
💬 hodlinator commented on pull request "headerssync: Keep tests ahead of increasing params":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2115856000)
If we didn't succeed, then `request_more` shouldn't matter, but agree it's best to have it be false (just like `pow_validated_headers` should be empty). It would be better if `ProcessNextHeaders` returned something like `std::optional<ProcessingResult>` instead of having a `bool` field to indicate success, but this PR already feels large enough.
"foiled" means the attack attempt was detected and therefore failed. Elaborated the comment a bit in next push.
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2115856000)
If we didn't succeed, then `request_more` shouldn't matter, but agree it's best to have it be false (just like `pow_validated_headers` should be empty). It would be better if `ProcessNextHeaders` returned something like `std::optional<ProcessingResult>` instead of having a `bool` field to indicate success, but this PR already feels large enough.
"foiled" means the attack attempt was detected and therefore failed. Elaborated the comment a bit in next push.
💬 hodlinator commented on pull request "headerssync: Keep tests ahead of increasing params":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2115831187)
One could argue at least the `uint64_t`-taking ctor here and in `arith_uint256` should be explicit, but I'd rather not have to change src/pow.cpp in this PR.
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2115831187)
One could argue at least the `uint64_t`-taking ctor here and in `arith_uint256` should be explicit, but I'd rather not have to change src/pow.cpp in this PR.
💬 hodlinator commented on pull request "headerssync: Keep tests ahead of increasing params":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2116586810)
Took the scoping idea, still not sure about the structured bindings.
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2116586810)
Took the scoping idea, still not sure about the structured bindings.
💬 hodlinator commented on pull request "headerssync: Keep tests ahead of increasing params":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2115807522)
What's the point of making a constructor that takes no arguments `explicit`?
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2115807522)
What's the point of making a constructor that takes no arguments `explicit`?
💬 hodlinator commented on pull request "headerssync: Keep tests ahead of increasing params":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2115857753)
Agree on the split but what would be the point of changing the type?
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2115857753)
Agree on the split but what would be the point of changing the type?
💬 hodlinator commented on pull request "headerssync: Keep tests ahead of increasing params":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2115879398)
The new comment is replacing the first (mention of python file) and last comments here:
https://github.com/bitcoin/bitcoin/blob/11a2d3a63e90cdc1920ede3c67d52a9c72860e6b/src/headerssync.cpp#L12-L25
Your comment would probably be enough but I'd rather keep some more context.
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2115879398)
The new comment is replacing the first (mention of python file) and last comments here:
https://github.com/bitcoin/bitcoin/blob/11a2d3a63e90cdc1920ede3c67d52a9c72860e6b/src/headerssync.cpp#L12-L25
Your comment would probably be enough but I'd rather keep some more context.
💬 hodlinator commented on pull request "headerssync: Keep tests ahead of increasing params":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2115884389)
Thanks, missed that!
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2115884389)
Thanks, missed that!
💬 hodlinator commented on pull request "headerssync: Keep tests ahead of increasing params":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2116580384)
Started renaming but then realized that we already have *headerssync-params.py*. It's not obvious to me that underscores would be more correct for C++ files. We do have other C++ files with hyphens.
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2116580384)
Started renaming but then realized that we already have *headerssync-params.py*. It's not obvious to me that underscores would be more correct for C++ files. We do have other C++ files with hyphens.
💬 hodlinator commented on pull request "headerssync: Keep tests ahead of increasing params":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2116614226)
Hm.. my `std::optional<ProcessingResult>` suggestion would not work here:
https://github.com/bitcoin/bitcoin/blob/a7b9cc4a0af0dbe4031ec5848d403f61dc9c3356/src/test/headers_sync_chainwork_tests.cpp#L127-L136
Success seems to mean that PRESYNC- and REDOWNLOAD-headers matched, even if the chain is too short.
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2116614226)
Hm.. my `std::optional<ProcessingResult>` suggestion would not work here:
https://github.com/bitcoin/bitcoin/blob/a7b9cc4a0af0dbe4031ec5848d403f61dc9c3356/src/test/headers_sync_chainwork_tests.cpp#L127-L136
Success seems to mean that PRESYNC- and REDOWNLOAD-headers matched, even if the chain is too short.
🤔 hebasto reviewed a pull request: "deps: Bump lief to 0.16.6"
(https://github.com/bitcoin/bitcoin/pull/32431#pullrequestreview-2882489162)
`ninja` is already [listed](https://github.com/bitcoin/bitcoin/blob/4b1d48a6866b24f0ed027334c6de642fc848d083/contrib/guix/manifest.scm#L15) as a dependency in `manifest.scm`, so patching it out doesn't seem necessary:
```diff
--- a/contrib/guix/manifest.scm
+++ b/contrib/guix/manifest.scm
@@ -167,21 +167,13 @@ chain for " target " development."))
(url "https://github.com/lief-project/LIEF")
(commit version)))
(file-name (git-file-na
...
(https://github.com/bitcoin/bitcoin/pull/32431#pullrequestreview-2882489162)
`ninja` is already [listed](https://github.com/bitcoin/bitcoin/blob/4b1d48a6866b24f0ed027334c6de642fc848d083/contrib/guix/manifest.scm#L15) as a dependency in `manifest.scm`, so patching it out doesn't seem necessary:
```diff
--- a/contrib/guix/manifest.scm
+++ b/contrib/guix/manifest.scm
@@ -167,21 +167,13 @@ chain for " target " development."))
(url "https://github.com/lief-project/LIEF")
(commit version)))
(file-name (git-file-na
...
💬 fanquake commented on pull request "cmake, qt: Process `*.qrc` files manually":
(https://github.com/bitcoin/bitcoin/pull/32648#issuecomment-2923517649)
Thanks for taking a look at this. I'm not sure the complexity of having to manually process, and then still having to supress warnings anyway, is worth it. I was thinking the fix here would be to change the tools / Qt to just produce files that don't have the issue. Seems like something we could fix upstream?
(https://github.com/bitcoin/bitcoin/pull/32648#issuecomment-2923517649)
Thanks for taking a look at this. I'm not sure the complexity of having to manually process, and then still having to supress warnings anyway, is worth it. I was thinking the fix here would be to change the tools / Qt to just produce files that don't have the issue. Seems like something we could fix upstream?
📝 hebasto opened a pull request: "cmake: Replace deprecated `qt6_add_translation` with `qt6_add_lrelease`"
(https://github.com/bitcoin/bitcoin/pull/32651)
See Qt docs:
- https://doc.qt.io/archives/qt-6.2/qtlinguist-cmake-qt-add-translation.html
- https://doc.qt.io/archives/qt-6.2/qtlinguist-cmake-qt-add-lrelease.html
(https://github.com/bitcoin/bitcoin/pull/32651)
See Qt docs:
- https://doc.qt.io/archives/qt-6.2/qtlinguist-cmake-qt-add-translation.html
- https://doc.qt.io/archives/qt-6.2/qtlinguist-cmake-qt-add-lrelease.html
💬 fanquake commented on pull request "cmake: Replace deprecated `qt6_add_translation` with `qt6_add_lrelease`":
(https://github.com/bitcoin/bitcoin/pull/32651#discussion_r2116704527)
If we're touching it already, can we fix the TODO, rather than move it? Or, put it into an issue, so that someone might pick it up?
(https://github.com/bitcoin/bitcoin/pull/32651#discussion_r2116704527)
If we're touching it already, can we fix the TODO, rather than move it? Or, put it into an issue, so that someone might pick it up?
📝 roconnor-blockstream opened a pull request: "wallet: add codex32 argument to addhdkey"
(https://github.com/bitcoin/bitcoin/pull/32652)
Rebase of #27351 onto #29136.
WIP. Testcases don't pass yet.
This PR introduces the ability to import [BIP 93/codex32](https://github.com/bitcoin/bips/blob/master/bip-0093.mediawiki) master seeds with the `addhdkey` command. It currently expects seeds to be provided in either as a single seed or as a list of shares which can be assembled via Shamir Secret Sharing.
(https://github.com/bitcoin/bitcoin/pull/32652)
Rebase of #27351 onto #29136.
WIP. Testcases don't pass yet.
This PR introduces the ability to import [BIP 93/codex32](https://github.com/bitcoin/bips/blob/master/bip-0093.mediawiki) master seeds with the `addhdkey` command. It currently expects seeds to be provided in either as a single seed or as a list of shares which can be assembled via Shamir Secret Sharing.
✅ pinheadmz closed an issue: "Bitcoin frog"
(https://github.com/bitcoin/bitcoin/issues/32221)
(https://github.com/bitcoin/bitcoin/issues/32221)
💬 ajtowns commented on pull request "p2p: Drop unsolicited CMPCTBLOCK from non-HB peer":
(https://github.com/bitcoin/bitcoin/pull/32606#issuecomment-2923667684)
> My main concern here is if this disables a FIBRE future; FIBRE apparently used unsolicited compact blocks
The fact that you're enabling FIBRE on a connection can just be taken as implicitly soliciting compact blocks on that connection, so I don't think this is a problem.
(https://github.com/bitcoin/bitcoin/pull/32606#issuecomment-2923667684)
> My main concern here is if this disables a FIBRE future; FIBRE apparently used unsolicited compact blocks
The fact that you're enabling FIBRE on a connection can just be taken as implicitly soliciting compact blocks on that connection, so I don't think this is a problem.
💬 hodlinator commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2923670457)
re-ACK a189d636184b1c28fa4a325b56c1fab8f44527b1
Changes since previous review (https://github.com/bitcoin/bitcoin/pull/32406#pullrequestreview-2870311100):
* Resolved silent merge conflict with 47894367b583c06c53d568dc4a984d27bac8f742 by same author.
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2923670457)
re-ACK a189d636184b1c28fa4a325b56c1fab8f44527b1
Changes since previous review (https://github.com/bitcoin/bitcoin/pull/32406#pullrequestreview-2870311100):
* Resolved silent merge conflict with 47894367b583c06c53d568dc4a984d27bac8f742 by same author.
💬 hebasto commented on pull request "cmake: Replace deprecated `qt6_add_translation` with `qt6_add_lrelease`":
(https://github.com/bitcoin/bitcoin/pull/32651#issuecomment-2924462495)
My Guix build:
```
aarch64
58c6cdfd914bfd2f2646640166692a0a02ec515809d5fa05cb3c9ff511617958 guix-build-d73400e06bef/output/aarch64-linux-gnu/SHA256SUMS.part
51fdf3f3ddb16b5aaf571f33f0da5b95f0b728c7d07863df48dfd3c2d02480dc guix-build-d73400e06bef/output/aarch64-linux-gnu/bitcoin-d73400e06bef-aarch64-linux-gnu-debug.tar.gz
cd2198dbde7ea6af923cc24c4a7363a3355e18f14a397994684db3c1b1662108 guix-build-d73400e06bef/output/aarch64-linux-gnu/bitcoin-d73400e06bef-aarch64-linux-gnu.tar.gz
855c93e3
...
(https://github.com/bitcoin/bitcoin/pull/32651#issuecomment-2924462495)
My Guix build:
```
aarch64
58c6cdfd914bfd2f2646640166692a0a02ec515809d5fa05cb3c9ff511617958 guix-build-d73400e06bef/output/aarch64-linux-gnu/SHA256SUMS.part
51fdf3f3ddb16b5aaf571f33f0da5b95f0b728c7d07863df48dfd3c2d02480dc guix-build-d73400e06bef/output/aarch64-linux-gnu/bitcoin-d73400e06bef-aarch64-linux-gnu-debug.tar.gz
cd2198dbde7ea6af923cc24c4a7363a3355e18f14a397994684db3c1b1662108 guix-build-d73400e06bef/output/aarch64-linux-gnu/bitcoin-d73400e06bef-aarch64-linux-gnu.tar.gz
855c93e3
...
⚠️ hebasto opened an issue: "cmake: Replace f`ile(GLOB ...)` command with an explicit list of `*.ts` files"
(https://github.com/bitcoin/bitcoin/issues/32653)
From CMake [docs](https://cmake.org/cmake/help/latest/command/file.html#glob):
> We do not recommend using GLOB to collect a list of source files from your source tree. If no CMakeLists.txt file changes when a source is added or removed then the generated build system cannot know when to ask CMake to regenerate.
So the following case have to be reworked:https://github.com/bitcoin/bitcoin/blob/4b1d48a6866b24f0ed027334c6de642fc848d083/src/qt/CMakeLists.txt#L57-L60
(https://github.com/bitcoin/bitcoin/issues/32653)
From CMake [docs](https://cmake.org/cmake/help/latest/command/file.html#glob):
> We do not recommend using GLOB to collect a list of source files from your source tree. If no CMakeLists.txt file changes when a source is added or removed then the generated build system cannot know when to ask CMake to regenerate.
So the following case have to be reworked:https://github.com/bitcoin/bitcoin/blob/4b1d48a6866b24f0ed027334c6de642fc848d083/src/qt/CMakeLists.txt#L57-L60