💬 KaueTech commented on pull request "Add Docker support with multi-arch build and user permissions handling":
(https://github.com/bitcoin/bitcoin/pull/33139#discussion_r2257180805)
> I don't think it makes sense to enumerate them or even mention them. It should simply use the native arch. This can be achieved by just passing `--platform=linux`, like in the CI system
The TARGETARCH is only for the image build that downloads from bitcoincore.net, because it’s needed in the wget URL:
https://bitcoincore.org/bin/bitcoin-core-${VERSION}/bitcoin-${VERSION}-${ARCH}-linux-gnu.tar.gz
The image build from local code doesn’t need VERSION or ARCH.
The build from GitHub code
...
(https://github.com/bitcoin/bitcoin/pull/33139#discussion_r2257180805)
> I don't think it makes sense to enumerate them or even mention them. It should simply use the native arch. This can be achieved by just passing `--platform=linux`, like in the CI system
The TARGETARCH is only for the image build that downloads from bitcoincore.net, because it’s needed in the wget URL:
https://bitcoincore.org/bin/bitcoin-core-${VERSION}/bitcoin-${VERSION}-${ARCH}-linux-gnu.tar.gz
The image build from local code doesn’t need VERSION or ARCH.
The build from GitHub code
...
💬 sipa commented on pull request "Introduce per-txin sighash midstate cache for legacy/p2sh/segwitv0 scripts":
(https://github.com/bitcoin/bitcoin/pull/32473#discussion_r2257185685)
Nice catch. Seems your fuzz test below (now included here) catches this.
(https://github.com/bitcoin/bitcoin/pull/32473#discussion_r2257185685)
Nice catch. Seems your fuzz test below (now included here) catches this.
💬 sipa commented on pull request "Introduce per-txin sighash midstate cache for legacy/p2sh/segwitv0 scripts":
(https://github.com/bitcoin/bitcoin/pull/32473#discussion_r2257192024)
Maybe there are currently no tests that actually make use of this, but it's still correct: it's not illegal to have a signature with sighash type 0 in non-taproot, and it would need to be serialized as an actual 0 at the end.
(https://github.com/bitcoin/bitcoin/pull/32473#discussion_r2257192024)
Maybe there are currently no tests that actually make use of this, but it's still correct: it's not illegal to have a signature with sighash type 0 in non-taproot, and it would need to be serialized as an actual 0 at the end.
💬 sipa commented on pull request "Introduce per-txin sighash midstate cache for legacy/p2sh/segwitv0 scripts":
(https://github.com/bitcoin/bitcoin/pull/32473#discussion_r2257193373)
Good point. Gone.
(https://github.com/bitcoin/bitcoin/pull/32473#discussion_r2257193373)
Good point. Gone.
💬 sipa commented on pull request "Introduce per-txin sighash midstate cache for legacy/p2sh/segwitv0 scripts":
(https://github.com/bitcoin/bitcoin/pull/32473#issuecomment-3160247633)
@theuni
> so the consensus checks are effectively using the new sigcache midstate caches. Or am I missing something?
Good point. I think that further undermines the idea of disabling it for consensus-critical checks, as even though it's possible to set the flags in such a way that any script-execution-cache-storing paths also disable sighash midstate caches, that makes it even less obvious to readers.
@achow101
> I would prefer having the cache always active as I find that much eas
...
(https://github.com/bitcoin/bitcoin/pull/32473#issuecomment-3160247633)
@theuni
> so the consensus checks are effectively using the new sigcache midstate caches. Or am I missing something?
Good point. I think that further undermines the idea of disabling it for consensus-critical checks, as even though it's possible to set the flags in such a way that any script-execution-cache-storing paths also disable sighash midstate caches, that makes it even less obvious to readers.
@achow101
> I would prefer having the cache always active as I find that much eas
...
💬 Crypt-iQ commented on pull request "validation: detect witness stripping without re-running Script checks":
(https://github.com/bitcoin/bitcoin/pull/33105#discussion_r2257229087)
> So I think my point is the opposite -- we should check for witness stripped early, and not bother very much with doing the other checks even if they're easy.
I agree.
I double-checked and the current code does not change behavior.
The early check in an earlier iteration of this PR changed behavior in two ways:
- consensus-invalid for non-witness reasons could be converted to witness-stripped, but this does not matter as they are consensus-invalid and a malicious peer can't interfere
...
(https://github.com/bitcoin/bitcoin/pull/33105#discussion_r2257229087)
> So I think my point is the opposite -- we should check for witness stripped early, and not bother very much with doing the other checks even if they're easy.
I agree.
I double-checked and the current code does not change behavior.
The early check in an earlier iteration of this PR changed behavior in two ways:
- consensus-invalid for non-witness reasons could be converted to witness-stripped, but this does not matter as they are consensus-invalid and a malicious peer can't interfere
...
📝 fanquake converted_to_draft a pull request: "truc: optimize the in package relation calculation"
(https://github.com/bitcoin/bitcoin/pull/33062)
In PackageTRUCChecks(), we calculate the in-package-parents for an in package tx, which results in the total time complexisity being O(n^2), n is the number of Txs in the package. Let's precompute the overall relationships between all transactions to make it be O(n).
(https://github.com/bitcoin/bitcoin/pull/33062)
In PackageTRUCChecks(), we calculate the in-package-parents for an in package tx, which results in the total time complexisity being O(n^2), n is the number of Txs in the package. Let's precompute the overall relationships between all transactions to make it be O(n).
🚀 fanquake merged a pull request: "contrib: drop use of `PermissionsStartOnly` & `Group=`"
(https://github.com/bitcoin/bitcoin/pull/33044)
(https://github.com/bitcoin/bitcoin/pull/33044)
👍 stickies-v approved a pull request: "log: rate limiting followups"
(https://github.com/bitcoin/bitcoin/pull/33011#pullrequestreview-3092416436)
ACK 8dd5c0a0447bbb6fe597aabfa725b397276da166
It's a debug-only option, so I suppose not technically a blocker, but would be best to change the `-disableratelimitlogging` option before merging.
(https://github.com/bitcoin/bitcoin/pull/33011#pullrequestreview-3092416436)
ACK 8dd5c0a0447bbb6fe597aabfa725b397276da166
It's a debug-only option, so I suppose not technically a blocker, but would be best to change the `-disableratelimitlogging` option before merging.
💬 stickies-v commented on pull request "log: rate limiting followups":
(https://github.com/bitcoin/bitcoin/pull/33011#discussion_r2257061642)
This is a bit brittle, e.g. if started with `-disableratelimitlogging=0` this would still disable ratelimiting. I'd suggest using the actual boolean value rather than checking if the argument is set. Also, ArgsManager has a built-in negation system: arguments also have a "no{arg}" option to disable it, which currently would be `-nodisableratelimitlogging` which is getting quite verbose.
Would prefer keeping options positive, and switch to `-{no}logratelimit` instead, keeping naming in line wi
...
(https://github.com/bitcoin/bitcoin/pull/33011#discussion_r2257061642)
This is a bit brittle, e.g. if started with `-disableratelimitlogging=0` this would still disable ratelimiting. I'd suggest using the actual boolean value rather than checking if the argument is set. Also, ArgsManager has a built-in negation system: arguments also have a "no{arg}" option to disable it, which currently would be `-nodisableratelimitlogging` which is getting quite verbose.
Would prefer keeping options positive, and switch to `-{no}logratelimit` instead, keeping naming in line wi
...
💬 stickies-v commented on pull request "log: rate limiting followups":
(https://github.com/bitcoin/bitcoin/pull/33011#discussion_r2257108689)
Probably good to add this to release notes?
<details>
<summary>git diff on 8dd5c0a044</summary>
```diff
diff --git a/doc/release-notes-32604.md b/doc/release-notes-32604.md
index d4d3726e86..1aaebc185b 100644
--- a/doc/release-notes-32604.md
+++ b/doc/release-notes-32604.md
@@ -4,7 +4,8 @@ Unconditional logging to disk is now rate limited by giving each source location
a quota of 1MiB per hour. Unconditional logging is any logging with a log level
higher than debug, that is `info
...
(https://github.com/bitcoin/bitcoin/pull/33011#discussion_r2257108689)
Probably good to add this to release notes?
<details>
<summary>git diff on 8dd5c0a044</summary>
```diff
diff --git a/doc/release-notes-32604.md b/doc/release-notes-32604.md
index d4d3726e86..1aaebc185b 100644
--- a/doc/release-notes-32604.md
+++ b/doc/release-notes-32604.md
@@ -4,7 +4,8 @@ Unconditional logging to disk is now rate limited by giving each source location
a quota of 1MiB per hour. Unconditional logging is any logging with a log level
higher than debug, that is `info
...
💬 KaueTech commented on pull request "Add Docker support with multi-arch build and user permissions handling":
(https://github.com/bitcoin/bitcoin/pull/33139#issuecomment-3160321027)
> Concept NACK. The benefits of this are unclear, this seems to just add a new dependency/abstraction layer (for development?), without solving any specific problem. It's also trying to do three things at once, and there already exist maintained Dockerfiles, for the deployment scenario, i.e https://github.com/willcl-ark/bitcoin-core-docker.
>
> > developers can edit the Dockerfile or mount their source with custom flags.
>
> This is more work than just using CMake and setting options?
>
...
(https://github.com/bitcoin/bitcoin/pull/33139#issuecomment-3160321027)
> Concept NACK. The benefits of this are unclear, this seems to just add a new dependency/abstraction layer (for development?), without solving any specific problem. It's also trying to do three things at once, and there already exist maintained Dockerfiles, for the deployment scenario, i.e https://github.com/willcl-ark/bitcoin-core-docker.
>
> > developers can edit the Dockerfile or mount their source with custom flags.
>
> This is more work than just using CMake and setting options?
>
...
💬 fanquake commented on pull request "Add Docker support with multi-arch build and user permissions handling":
(https://github.com/bitcoin/bitcoin/pull/33139#issuecomment-3160332666)
> not maintained by Bitcoin Core contributors,
It is. @willcl-ark is a regular contributor to the project.
(https://github.com/bitcoin/bitcoin/pull/33139#issuecomment-3160332666)
> not maintained by Bitcoin Core contributors,
It is. @willcl-ark is a regular contributor to the project.
💬 KaueTech commented on pull request "Add Docker support with multi-arch build and user permissions handling":
(https://github.com/bitcoin/bitcoin/pull/33139#issuecomment-3160342710)
> > not maintained by Bitcoin Core contributors,
>
> It is. @willcl-ark is a regular contributor to the project.
I’m not saying he’s not a trustworthy person — just that his work isn’t in the Bitcoin Core repository. I think everyone naturally prefers what’s in the official repo, since any code inside it carries more trust from the community.
But as I said, it’s nothing strictly necessary.
(https://github.com/bitcoin/bitcoin/pull/33139#issuecomment-3160342710)
> > not maintained by Bitcoin Core contributors,
>
> It is. @willcl-ark is a regular contributor to the project.
I’m not saying he’s not a trustworthy person — just that his work isn’t in the Bitcoin Core repository. I think everyone naturally prefers what’s in the official repo, since any code inside it carries more trust from the community.
But as I said, it’s nothing strictly necessary.
💬 sipa commented on pull request "Add Docker support with multi-arch build and user permissions handling":
(https://github.com/bitcoin/bitcoin/pull/33139#issuecomment-3160353283)
FWIW, I think there is value in having some form of docker integration *for users*, as part of the repository/release itself and not just maintained by a third party. I'm not familiar enough with docker myself to judge what approaches are appropriate, but it's a sufficiently common request that I think it would be nice if Bitcoin Core offered something like this. But, obviously, only if someone wants to step to maintain it, and make sure infrastructure is available to test it doesn't bitrot.
Fo
...
(https://github.com/bitcoin/bitcoin/pull/33139#issuecomment-3160353283)
FWIW, I think there is value in having some form of docker integration *for users*, as part of the repository/release itself and not just maintained by a third party. I'm not familiar enough with docker myself to judge what approaches are appropriate, but it's a sufficiently common request that I think it would be nice if Bitcoin Core offered something like this. But, obviously, only if someone wants to step to maintain it, and make sure infrastructure is available to test it doesn't bitrot.
Fo
...
💬 willcl-ark commented on pull request "test: Run bench sanity checks in parallel with functional tests":
(https://github.com/bitcoin/bitcoin/pull/33142#issuecomment-3160374012)
I get an error compiling this without `bench_bitcoin`:
```
❯ cmake -B build; and cmake --build build; and ./build/test/functional/test_runner.py -j14
-- The CXX compiler identification is Clang 19.1.7
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /etc/profiles/per-user/will/bin/c++ - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Setting build type to "RelWithDebInfo" as none was s
...
(https://github.com/bitcoin/bitcoin/pull/33142#issuecomment-3160374012)
I get an error compiling this without `bench_bitcoin`:
```
❯ cmake -B build; and cmake --build build; and ./build/test/functional/test_runner.py -j14
-- The CXX compiler identification is Clang 19.1.7
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /etc/profiles/per-user/will/bin/c++ - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Setting build type to "RelWithDebInfo" as none was s
...
📝 maflcko opened a pull request: "build: Set AUTHOR_WARNING on warnings"
(https://github.com/bitcoin/bitcoin/pull/33144)
Now that the cmake setting `-Werror=dev` is set since commit 6a13a6106e3c1ebe95ba6430184d6260a7b942bd for the CI, guix and the dev cmake preset, it could make sense to notify developers about any warnings.
So do that with a single `AUTHOR_WARNING`.
This can be tested by introducing a bug, like:
```diff
diff --git a/CMakeLists.txt b/CMakeLists.txt
index 6017775fa7..5610e03c66 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -589,7 +589,7 @@ set(Python3_FIND_FRAMEWORK LAST CACHE S
...
(https://github.com/bitcoin/bitcoin/pull/33144)
Now that the cmake setting `-Werror=dev` is set since commit 6a13a6106e3c1ebe95ba6430184d6260a7b942bd for the CI, guix and the dev cmake preset, it could make sense to notify developers about any warnings.
So do that with a single `AUTHOR_WARNING`.
This can be tested by introducing a bug, like:
```diff
diff --git a/CMakeLists.txt b/CMakeLists.txt
index 6017775fa7..5610e03c66 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -589,7 +589,7 @@ set(Python3_FIND_FRAMEWORK LAST CACHE S
...
💬 maflcko commented on pull request "cmake: Use `AUTHOR_WARNING` for warnings":
(https://github.com/bitcoin/bitcoin/pull/32865#discussion_r2257379206)
Done in https://github.com/bitcoin/bitcoin/pull/33144. It should be trivial to revert that one-line patch and rebase this pull on top of it, if there is need.
(https://github.com/bitcoin/bitcoin/pull/32865#discussion_r2257379206)
Done in https://github.com/bitcoin/bitcoin/pull/33144. It should be trivial to revert that one-line patch and rebase this pull on top of it, if there is need.
💬 maflcko commented on pull request "cmake: Improve Python robustness and test usability":
(https://github.com/bitcoin/bitcoin/pull/31233#discussion_r2257425905)
> > (Actually, while testing, a missing python is now a hard error already when `-DBUILD_GUI=ON`. So I don't think the wording "refactor" accurately represents the changes here.)
>
> To clarify, either the changes here in this pull need to be reverted/fixed up, or the docs should be updated to clarify that python is now required to build the gui.
This bug still exists. `rm -rf ./bld-cmake && cmake -B ./bld-cmake -DBUILD_GUI=ON` now fails with:
```
CMake Error at src/qt/CMakeLists.txt:3
...
(https://github.com/bitcoin/bitcoin/pull/31233#discussion_r2257425905)
> > (Actually, while testing, a missing python is now a hard error already when `-DBUILD_GUI=ON`. So I don't think the wording "refactor" accurately represents the changes here.)
>
> To clarify, either the changes here in this pull need to be reverted/fixed up, or the docs should be updated to clarify that python is now required to build the gui.
This bug still exists. `rm -rf ./bld-cmake && cmake -B ./bld-cmake -DBUILD_GUI=ON` now fails with:
```
CMake Error at src/qt/CMakeLists.txt:3
...
💬 maflcko commented on pull request "Add Docker support with multi-arch build and user permissions handling":
(https://github.com/bitcoin/bitcoin/pull/33139#discussion_r2257455023)
It should just auto-detect, if it is only used internally? Seems odd to ask the user to lookup their arch and pass a magic value?
(https://github.com/bitcoin/bitcoin/pull/33139#discussion_r2257455023)
It should just auto-detect, if it is only used internally? Seems odd to ask the user to lookup their arch and pass a magic value?