📝 fanquake opened a pull request: "[28.x] Backports "
(https://github.com/bitcoin/bitcoin/pull/33143)
Backports:
* #33133
(https://github.com/bitcoin/bitcoin/pull/33143)
Backports:
* #33133
💬 fanquake commented on pull request "ci: Remove redundant RUN_UNIT_TESTS_SEQUENTIAL":
(https://github.com/bitcoin/bitcoin/pull/33136#discussion_r2256836237)
> to catch test case failures caused by dirty global state from prior test cases.
If the aim is to catch dirty state, should we run it in a job with the most unit tests? Windows cross-build is going to run less than Linux.
(https://github.com/bitcoin/bitcoin/pull/33136#discussion_r2256836237)
> to catch test case failures caused by dirty global state from prior test cases.
If the aim is to catch dirty state, should we run it in a job with the most unit tests? Windows cross-build is going to run less than Linux.
💬 maflcko commented on pull request "ci: Remove redundant RUN_UNIT_TESTS_SEQUENTIAL":
(https://github.com/bitcoin/bitcoin/pull/33136#discussion_r2256852597)
I think it is always possible to miss unit tests that are not compiled. For the Windows ones, it should only be a few about windows paths, fork(), and socketpair, which seems fine to skip.
If there is need to run it in another task, it can be done as a follow-up?
(https://github.com/bitcoin/bitcoin/pull/33136#discussion_r2256852597)
I think it is always possible to miss unit tests that are not compiled. For the Windows ones, it should only be a few about windows paths, fork(), and socketpair, which seems fine to skip.
If there is need to run it in another task, it can be done as a follow-up?
💬 cedwies commented on pull request "tests: cover abortrescan() in-flight True path with dynamic-tail retry":
(https://github.com/bitcoin/bitcoin/pull/33131#discussion_r2256883474)
I currently don't see a way around this. Any thoughts on how one could solve this problem?
(https://github.com/bitcoin/bitcoin/pull/33131#discussion_r2256883474)
I currently don't see a way around this. Any thoughts on how one could solve this problem?
🚀 fanquake merged a pull request: "kernel: create monolithic kernel static library"
(https://github.com/bitcoin/bitcoin/pull/33077)
(https://github.com/bitcoin/bitcoin/pull/33077)
💬 fanquake commented on pull request "rpc: fix getpeerinfo ping duration unit docs":
(https://github.com/bitcoin/bitcoin/pull/33133#issuecomment-3159907692)
Backported to `28.x` in #33143.
(https://github.com/bitcoin/bitcoin/pull/33133#issuecomment-3159907692)
Backported to `28.x` in #33143.
💬 fanquake commented on pull request "Interfaces: Expose UTXO Snapshot Loading and Add Progress Notifications":
(https://github.com/bitcoin/bitcoin/pull/33117#issuecomment-3160117477)
https://github.com/bitcoin/bitcoin/actions/runs/16728433539/job/47504424705?pr=33117#step:10:445:
```bash
D:\a\bitcoin\bitcoin\src\node\interfaces.cpp(115,40): error C2220: the following warning is treated as an error [D:\a\bitcoin\bitcoin\build\src\bitcoin_node.vcxproj]
D:\a\bitcoin\bitcoin\src\node\interfaces.cpp(115,40): warning C4101: 'e': unreferenced local variable [D:\a\bitcoin\bitcoin\build\src\bitcoin_node.vcxproj]
```
(https://github.com/bitcoin/bitcoin/pull/33117#issuecomment-3160117477)
https://github.com/bitcoin/bitcoin/actions/runs/16728433539/job/47504424705?pr=33117#step:10:445:
```bash
D:\a\bitcoin\bitcoin\src\node\interfaces.cpp(115,40): error C2220: the following warning is treated as an error [D:\a\bitcoin\bitcoin\build\src\bitcoin_node.vcxproj]
D:\a\bitcoin\bitcoin\src\node\interfaces.cpp(115,40): warning C4101: 'e': unreferenced local variable [D:\a\bitcoin\bitcoin\build\src\bitcoin_node.vcxproj]
```
💬 willcl-ark commented on pull request "ci: Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#issuecomment-3160184649)
Force pushed to pick up changes in master (particularly #33002).
Other notable changes:
- Added a remote ccache host in 2adbe2caec. Previously we used actions/cache blobs for ccache. This works ~ ok, but Cirrus restores partial matches in reverse-age order (oldest first), which was resulting in bad cache hitrates for ccache when multiple caches existed for a job (say, "old" and "new"). The HTTP ccache backend is used on Cirrus infra and shared between all jobs. This requires using docker `-h
...
(https://github.com/bitcoin/bitcoin/pull/32989#issuecomment-3160184649)
Force pushed to pick up changes in master (particularly #33002).
Other notable changes:
- Added a remote ccache host in 2adbe2caec. Previously we used actions/cache blobs for ccache. This works ~ ok, but Cirrus restores partial matches in reverse-age order (oldest first), which was resulting in bad cache hitrates for ccache when multiple caches existed for a job (say, "old" and "new"). The HTTP ccache backend is used on Cirrus infra and shared between all jobs. This requires using docker `-h
...
💬 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?
>
...