💬 romanz commented on pull request "index: store per-block transaction locations for efficient lookups":
(https://github.com/bitcoin/bitcoin/pull/32541#issuecomment-3150541294)
Applied a few more fixes: [`b2a22ce -> 4441827`](https://github.com/bitcoin/bitcoin/compare/b2a22ce33dc69697181547fa1e83bd0ed3321565..4441827ef4e9b5fe306c5f0a81a52b5d2b5e0b69)
(https://github.com/bitcoin/bitcoin/pull/32541#issuecomment-3150541294)
Applied a few more fixes: [`b2a22ce -> 4441827`](https://github.com/bitcoin/bitcoin/compare/b2a22ce33dc69697181547fa1e83bd0ed3321565..4441827ef4e9b5fe306c5f0a81a52b5d2b5e0b69)
💬 hodlinator commented on pull request "Allowing multi client support in guix-build":
(https://github.com/bitcoin/bitcoin/pull/33126#discussion_r2251460522)
Might be able to parse `CLIENT_NAME` from *build/src/bitcoin-build-config.h* and replace ` ` with `-`?
(https://github.com/bitcoin/bitcoin/pull/33126#discussion_r2251460522)
Might be able to parse `CLIENT_NAME` from *build/src/bitcoin-build-config.h* and replace ` ` with `-`?
🤔 glozow reviewed a pull request: "fuzz: txgraph: fix `real_is_optimal` flag propagation in `CommitStaging`"
(https://github.com/bitcoin/bitcoin/pull/33132#pullrequestreview-3084265420)
ACK 444dcb2f9944ad5208bf00c9f197da7b2c98063c
The fix works and makes sense to me. I suppose a concrete scenario is:
1. we have main and staging, neither optimally linearized
2. we create an observer for the main level (alternatively, it could be oversized)
3. we call DoWork() which returns true after optimally linearizing the staging level and skipping the main level
4. we delete the main level observer
5. we call CommitStaging. sims staging gets copied over.
6. even though staging was
...
(https://github.com/bitcoin/bitcoin/pull/33132#pullrequestreview-3084265420)
ACK 444dcb2f9944ad5208bf00c9f197da7b2c98063c
The fix works and makes sense to me. I suppose a concrete scenario is:
1. we have main and staging, neither optimally linearized
2. we create an observer for the main level (alternatively, it could be oversized)
3. we call DoWork() which returns true after optimally linearizing the staging level and skipping the main level
4. we delete the main level observer
5. we call CommitStaging. sims staging gets copied over.
6. even though staging was
...
👍 willcl-ark approved a pull request: "cmake: Install internal binaries to <prefix>/libexec/"
(https://github.com/bitcoin/bitcoin/pull/31679#pullrequestreview-3084294348)
utACK f49840dd902cd9b14b6aadb431b16a4aeb719c3f
> Sorry about that, the PR description was confusing ...
No that was my bad, I should have read the PR comments before reviewing (perhaps, although I don't like to).
I am in favour of moving binaries to libexec/, including `test_bitcoin`, as per this change.
I've tried to find other bitcoin packages which may break on this, but can't find many, and the few I have found are already outdated in other ways, so require a "manual update" alre
...
(https://github.com/bitcoin/bitcoin/pull/31679#pullrequestreview-3084294348)
utACK f49840dd902cd9b14b6aadb431b16a4aeb719c3f
> Sorry about that, the PR description was confusing ...
No that was my bad, I should have read the PR comments before reviewing (perhaps, although I don't like to).
I am in favour of moving binaries to libexec/, including `test_bitcoin`, as per this change.
I've tried to find other bitcoin packages which may break on this, but can't find many, and the few I have found are already outdated in other ways, so require a "manual update" alre
...
💬 willcl-ark commented on pull request "Allowing multi client support in guix-build":
(https://github.com/bitcoin/bitcoin/pull/33126#issuecomment-3150797545)
Concept NACK
Unless this value was picked up from elsewhere, I don't see how this makes it easier to support "multi clients""? A two-line downstream patch... remains a two-line downstream patch. Only the lines being patched have changed?
(https://github.com/bitcoin/bitcoin/pull/33126#issuecomment-3150797545)
Concept NACK
Unless this value was picked up from elsewhere, I don't see how this makes it easier to support "multi clients""? A two-line downstream patch... remains a two-line downstream patch. Only the lines being patched have changed?
🤔 furszy reviewed a pull request: "wallet: Avoid potentially writing incorrect best block locator"
(https://github.com/bitcoin/bitcoin/pull/29652#pullrequestreview-3084332144)
Since #30221 got merged, I think this one can be closed
(https://github.com/bitcoin/bitcoin/pull/29652#pullrequestreview-3084332144)
Since #30221 got merged, I think this one can be closed
🤔 marcofleon reviewed a pull request: "p2p: TxOrphanage revamp cleanups"
(https://github.com/bitcoin/bitcoin/pull/32941#pullrequestreview-3084354759)
Code review ACK 3d4d4f0d92d42809e74377e4380abdc70f74de5d
Fuzzed all three targets over the weekend, no crashes. Only small thing I caught while reading the `txorphan_protected` target:
https://github.com/bitcoin/bitcoin/blob/3d4d4f0d92d42809e74377e4380abdc70f74de5d/src/test/fuzz/txorphan.cpp#L331
That line should be `(tx->vin.size() / 10) + 1` but it doesn't result in a crash because it's overestimating the latency score for an additional announcer.
(https://github.com/bitcoin/bitcoin/pull/32941#pullrequestreview-3084354759)
Code review ACK 3d4d4f0d92d42809e74377e4380abdc70f74de5d
Fuzzed all three targets over the weekend, no crashes. Only small thing I caught while reading the `txorphan_protected` target:
https://github.com/bitcoin/bitcoin/blob/3d4d4f0d92d42809e74377e4380abdc70f74de5d/src/test/fuzz/txorphan.cpp#L331
That line should be `(tx->vin.size() / 10) + 1` but it doesn't result in a crash because it's overestimating the latency score for an additional announcer.
💬 sipa commented on pull request "fuzz: txgraph: fix `real_is_optimal` flag propagation in `CommitStaging`":
(https://github.com/bitcoin/bitcoin/pull/33132#discussion_r2251601762)
Nit: if this is actually aiming to be forward compatible with a future more-than-two-levels extension (which we may in fact need at some point), I think this should be `sims.back(). ... = ...`. Doesn't matter now of course as it's all equivalent to level 0.
(https://github.com/bitcoin/bitcoin/pull/33132#discussion_r2251601762)
Nit: if this is actually aiming to be forward compatible with a future more-than-two-levels extension (which we may in fact need at some point), I think this should be `sims.back(). ... = ...`. Doesn't matter now of course as it's all equivalent to level 0.
💬 sipa commented on pull request "fuzz: txgraph: fix `real_is_optimal` flag propagation in `CommitStaging`":
(https://github.com/bitcoin/bitcoin/pull/33132#issuecomment-3150855612)
ACK 444dcb2f9944ad5208bf00c9f197da7b2c98063c
(https://github.com/bitcoin/bitcoin/pull/33132#issuecomment-3150855612)
ACK 444dcb2f9944ad5208bf00c9f197da7b2c98063c
✅ glozow closed an issue: "fuzz: `txgraph`: Assertion `cmp == 0' failed"
(https://github.com/bitcoin/bitcoin/issues/33097)
(https://github.com/bitcoin/bitcoin/issues/33097)
🚀 glozow merged a pull request: "fuzz: txgraph: fix `real_is_optimal` flag propagation in `CommitStaging`"
(https://github.com/bitcoin/bitcoin/pull/33132)
(https://github.com/bitcoin/bitcoin/pull/33132)
📝 0xB10C opened a pull request: "rpc: fix getpeerinfo ping duration unit docs"
(https://github.com/bitcoin/bitcoin/pull/33133)
The docs have been incorrect since a3789c700b5a43efd4b366b4241ae840d63f2349 (released in v25; master since Sept. 2022). Noticed while setting up monitoring using getpeerinfo.
https://github.com/bitcoin/bitcoin/blob/0cb1ed2b7c634754f4ad0a0083d7cf40989d7f48/src/rpc/net.cpp#L249-L257
<!--
*** 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/bitcoin/pull/33133)
The docs have been incorrect since a3789c700b5a43efd4b366b4241ae840d63f2349 (released in v25; master since Sept. 2022). Noticed while setting up monitoring using getpeerinfo.
https://github.com/bitcoin/bitcoin/blob/0cb1ed2b7c634754f4ad0a0083d7cf40989d7f48/src/rpc/net.cpp#L249-L257
<!--
*** 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
...
💬 0xB10C commented on pull request "rpc: fix getpeerinfo ping duration unit docs":
(https://github.com/bitcoin/bitcoin/pull/33133#issuecomment-3150986125)
It's correctly documented in the `ping` RPC docs https://github.com/bitcoin/bitcoin/blob/0cb1ed2b7c634754f4ad0a0083d7cf40989d7f48/src/rpc/net.cpp#L85-L88
(https://github.com/bitcoin/bitcoin/pull/33133#issuecomment-3150986125)
It's correctly documented in the `ping` RPC docs https://github.com/bitcoin/bitcoin/blob/0cb1ed2b7c634754f4ad0a0083d7cf40989d7f48/src/rpc/net.cpp#L85-L88
💬 maflcko commented on pull request "rpc: fix getpeerinfo ping duration unit docs":
(https://github.com/bitcoin/bitcoin/pull/33133#issuecomment-3151021429)
lgtm ACK 12df1a72831bf950c5d625b80c6fd6ef15b4117d
(https://github.com/bitcoin/bitcoin/pull/33133#issuecomment-3151021429)
lgtm ACK 12df1a72831bf950c5d625b80c6fd6ef15b4117d
💬 willcl-ark commented on pull request "ci: Use mlc `v1` and ignore `depends/work`":
(https://github.com/bitcoin/bitcoin/pull/33125#discussion_r2251648192)
Yes, the `--gitignore` flag is slightly buggy. I have fix this (and speeded up the entire run) by ignoring all files in ignore directores in this patch: https://github.com/becheran/mlc/compare/master...willcl-ark:mlc:ignore-dirs-find
Running that branch, with a dirty depends dir fixes that issue for me, although I still see one incorrect "error":
```
Result (1871 links):
OK 458
Skipped 1057
Warnings 355
Errors 1
The following links could not be resolved:
./src/secp2
...
(https://github.com/bitcoin/bitcoin/pull/33125#discussion_r2251648192)
Yes, the `--gitignore` flag is slightly buggy. I have fix this (and speeded up the entire run) by ignoring all files in ignore directores in this patch: https://github.com/becheran/mlc/compare/master...willcl-ark:mlc:ignore-dirs-find
Running that branch, with a dirty depends dir fixes that issue for me, although I still see one incorrect "error":
```
Result (1871 links):
OK 458
Skipped 1057
Warnings 355
Errors 1
The following links could not be resolved:
./src/secp2
...
💬 fanquake commented on pull request "ci: Use mlc `v1` and ignore `depends/work`":
(https://github.com/bitcoin/bitcoin/pull/33125#discussion_r2251720043)
> I suppose we should explicitly ignore our subtrees too then...
Aren't we already with `let mut md_ignore_paths = get_subtrees();` ?
(https://github.com/bitcoin/bitcoin/pull/33125#discussion_r2251720043)
> I suppose we should explicitly ignore our subtrees too then...
Aren't we already with `let mut md_ignore_paths = get_subtrees();` ?
💬 0xB10C commented on pull request "[29.x] Backports":
(https://github.com/bitcoin/bitcoin/pull/33074#issuecomment-3151100176)
#33133 probably too
(https://github.com/bitcoin/bitcoin/pull/33074#issuecomment-3151100176)
#33133 probably too
💬 glozow commented on pull request "p2p: TxOrphanage revamp cleanups":
(https://github.com/bitcoin/bitcoin/pull/32941#issuecomment-3151106136)
> That line should be (tx->vin.size() / 10) + 1
Thanks @marcofleon! Just added a commit on top to fix that.
(https://github.com/bitcoin/bitcoin/pull/32941#issuecomment-3151106136)
> That line should be (tx->vin.size() / 10) + 1
Thanks @marcofleon! Just added a commit on top to fix that.
💬 maflcko commented on issue "Revisiting us self-hosting parts of our CI":
(https://github.com/bitcoin/bitcoin/issues/31965#issuecomment-3151110988)
Hmm, so I looked into re-running GHA CI tasks to catch silent merge conflicts. However, GitHub does not allow this and blocks any re-run if the commit is older than 30 days. Even if the task has been recently re-run. Example: https://github.com/bitcoin/bitcoin/actions/runs/16022448902/job/47185933876?pr=32856 (can not be re-run anymore)
If the ability to re-run CI tasks is removed without replacement, this will likely lead to more pull requests being merged into master, where the CI fails.
The
...
(https://github.com/bitcoin/bitcoin/issues/31965#issuecomment-3151110988)
Hmm, so I looked into re-running GHA CI tasks to catch silent merge conflicts. However, GitHub does not allow this and blocks any re-run if the commit is older than 30 days. Even if the task has been recently re-run. Example: https://github.com/bitcoin/bitcoin/actions/runs/16022448902/job/47185933876?pr=32856 (can not be re-run anymore)
If the ability to re-run CI tasks is removed without replacement, this will likely lead to more pull requests being merged into master, where the CI fails.
The
...
🤔 jonatack reviewed a pull request: "rpc: fix getpeerinfo ping duration unit docs"
(https://github.com/bitcoin/bitcoin/pull/33133#pullrequestreview-3084685673)
ACK 12df1a72831bf950c5d625b80c6fd6ef15b4117d
(https://github.com/bitcoin/bitcoin/pull/33133#pullrequestreview-3084685673)
ACK 12df1a72831bf950c5d625b80c6fd6ef15b4117d