💬 fanquake commented on pull request "depends: strip when installing qt binaries":
(https://github.com/bitcoin/bitcoin/pull/33304#issuecomment-3254195129)
Guix Build (aarch64):
```bash
c805e4ee4a80b3b03919d280a32f272426811470646aca1e3da0a60255347738 guix-build-c9d5f211c119/output/aarch64-linux-gnu/SHA256SUMS.part
58f02ff3f18cd39e9379d0896680c9f7b6a3544a02362bb98570b95131f9d8cf guix-build-c9d5f211c119/output/aarch64-linux-gnu/bitcoin-c9d5f211c119-aarch64-linux-gnu-debug.tar.gz
5432eaaacb722bb998c2fe76e56e62ef728f60bc7fb27d50dd9ca08dc4648c09 guix-build-c9d5f211c119/output/aarch64-linux-gnu/bitcoin-c9d5f211c119-aarch64-linux-gnu.tar.gz
997f4c
...
(https://github.com/bitcoin/bitcoin/pull/33304#issuecomment-3254195129)
Guix Build (aarch64):
```bash
c805e4ee4a80b3b03919d280a32f272426811470646aca1e3da0a60255347738 guix-build-c9d5f211c119/output/aarch64-linux-gnu/SHA256SUMS.part
58f02ff3f18cd39e9379d0896680c9f7b6a3544a02362bb98570b95131f9d8cf guix-build-c9d5f211c119/output/aarch64-linux-gnu/bitcoin-c9d5f211c119-aarch64-linux-gnu-debug.tar.gz
5432eaaacb722bb998c2fe76e56e62ef728f60bc7fb27d50dd9ca08dc4648c09 guix-build-c9d5f211c119/output/aarch64-linux-gnu/bitcoin-c9d5f211c119-aarch64-linux-gnu.tar.gz
997f4c
...
💬 maflcko commented on pull request "ci: Checkout latest merged pulls":
(https://github.com/bitcoin/bitcoin/pull/33303#issuecomment-3254202893)
> Then perhaps encapsulate customized `checkout` action as well?
I don't think it matters much for other repos, but I am happy to push a commit, if someone writes one, or close this pull in favour of one that refactors this one :)
(https://github.com/bitcoin/bitcoin/pull/33303#issuecomment-3254202893)
> Then perhaps encapsulate customized `checkout` action as well?
I don't think it matters much for other repos, but I am happy to push a commit, if someone writes one, or close this pull in favour of one that refactors this one :)
💬 vasild commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#discussion_r2322561383)
Yeah, I thought about setting `-natpmp` to off, but decided would be better to test with the default arguments as that more closely matches the real world. That argument seems weak.
> band-aid more than a cure
I agree. Changed to what you suggest above.
(https://github.com/bitcoin/bitcoin/pull/31349#discussion_r2322561383)
Yeah, I thought about setting `-natpmp` to off, but decided would be better to test with the default arguments as that more closely matches the real world. That argument seems weak.
> band-aid more than a cure
I agree. Changed to what you suggest above.
💬 fqlx commented on pull request "rpc: require integer verbosity; remove boolean 'verbose'":
(https://github.com/bitcoin/bitcoin/pull/33214#discussion_r2322561424)
Moved this back into `rpc` namespace `server.cpp`
(https://github.com/bitcoin/bitcoin/pull/33214#discussion_r2322561424)
Moved this back into `rpc` namespace `server.cpp`
💬 vasild commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#issuecomment-3254248014)
`a9ac49c8ac...778675ac71`: pick https://github.com/bitcoin/bitcoin/pull/31349#discussion_r2322207556
(https://github.com/bitcoin/bitcoin/pull/31349#issuecomment-3254248014)
`a9ac49c8ac...778675ac71`: pick https://github.com/bitcoin/bitcoin/pull/31349#discussion_r2322207556
💬 ryanofsky commented on pull request "build: suggest -DENABLE_IPC=OFF when missing capnp":
(https://github.com/bitcoin/bitcoin/pull/33290#discussion_r2322584553)
> NACK to adding any depends specific code. `CMAKE_TOOLCHAIN_FILE` also doesn't necessarily mean a depends build; anyone can bring/use a toolchain file.
Yeah I think it is clear this is not a good long term solution, but to be fair I don't think there is much actual harm done here if CMAKE_TOOLCHAIN_FILE is set, since this would just fail to print a warning that is likely to be irrelevant.
But I do think it could be nice to close this PR and extend @willcl-ark's approach as described in ht
...
(https://github.com/bitcoin/bitcoin/pull/33290#discussion_r2322584553)
> NACK to adding any depends specific code. `CMAKE_TOOLCHAIN_FILE` also doesn't necessarily mean a depends build; anyone can bring/use a toolchain file.
Yeah I think it is clear this is not a good long term solution, but to be fair I don't think there is much actual harm done here if CMAKE_TOOLCHAIN_FILE is set, since this would just fail to print a warning that is likely to be irrelevant.
But I do think it could be nice to close this PR and extend @willcl-ark's approach as described in ht
...
🤔 mzumsande reviewed a pull request: "index: Force database compaction in coinstatsindex"
(https://github.com/bitcoin/bitcoin/pull/33306#pullrequestreview-3185973049)
Do you know if the current state with many small files and one large file has actual downsides, or does it just look weird?
I am not an expert on LevelDB, just mentioning that two AIs I consulted say that benefits of regular compaction would be only of cosmetic value and suggest to just let LevelDB do its thing.
(https://github.com/bitcoin/bitcoin/pull/33306#pullrequestreview-3185973049)
Do you know if the current state with many small files and one large file has actual downsides, or does it just look weird?
I am not an expert on LevelDB, just mentioning that two AIs I consulted say that benefits of regular compaction would be only of cosmetic value and suggest to just let LevelDB do its thing.
💬 maflcko commented on pull request "ci: disable cirrus cache in 32bit arm job":
(https://github.com/bitcoin/bitcoin/pull/33302#discussion_r2322672396)
hmm, still wrong https://github.com/bitcoin/bitcoin/actions/runs/17469260337/job/49613109539?pr=33302#step:5:7 ?
(https://github.com/bitcoin/bitcoin/pull/33302#discussion_r2322672396)
hmm, still wrong https://github.com/bitcoin/bitcoin/actions/runs/17469260337/job/49613109539?pr=33302#step:5:7 ?
💬 fjahr commented on pull request "index: Fix coinstats overflow":
(https://github.com/bitcoin/bitcoin/pull/30469#issuecomment-3254450772)
Dear reviewers, sorry for another slight change in approach here but I think this should be the last one and I think it makes reasoning about the review easier or at least not harder. Given the discussion about remaining (edge case) potential for overflows I have decided to switch the values we store to `arith_uint256`. This feels cleaner to me than dealing with overflows with additional code and a shared field etc. The nice side-effect of this is that this makes it feasible to keep the total va
...
(https://github.com/bitcoin/bitcoin/pull/30469#issuecomment-3254450772)
Dear reviewers, sorry for another slight change in approach here but I think this should be the last one and I think it makes reasoning about the review easier or at least not harder. Given the discussion about remaining (edge case) potential for overflows I have decided to switch the values we store to `arith_uint256`. This feels cleaner to me than dealing with overflows with additional code and a shared field etc. The nice side-effect of this is that this makes it feasible to keep the total va
...
💬 instagibbs commented on pull request "net: check for empty header before calling FillBlock":
(https://github.com/bitcoin/bitcoin/pull/33296#issuecomment-3254453984)
concept ACK, as per offline conversations. It would be really nice to make the state machine better, for now we can at least avoid the `Assume` in debug builds when a peer does this.
We could also disconnect the peer for this behavior, I think, but not a priority
(https://github.com/bitcoin/bitcoin/pull/33296#issuecomment-3254453984)
concept ACK, as per offline conversations. It would be really nice to make the state machine better, for now we can at least avoid the `Assume` in debug builds when a peer does this.
We could also disconnect the peer for this behavior, I think, but not a priority
💬 TheCharlatan commented on pull request "RFC: Riscv bare metal CI job":
(https://github.com/bitcoin/bitcoin/pull/31425#issuecomment-3254442149)
Rebased ec7c86c732c995d2c38e2a3f93ad55a451a8cf81 -> a577bbe5df766a4e0e5a36fc997b7880eec45c0e ([bare_metal_support_2](https://github.com/TheCharlatan/bitcoin/tree/bare_metal_support_2) -> [bare_metal_support_3](https://github.com/TheCharlatan/bitcoin/tree/bare_metal_support_3), [compare](https://github.com/TheCharlatan/bitcoin/compare/bare_metal_support_2..bare_metal_support_3))
* Fixed conflict with #32989
(https://github.com/bitcoin/bitcoin/pull/31425#issuecomment-3254442149)
Rebased ec7c86c732c995d2c38e2a3f93ad55a451a8cf81 -> a577bbe5df766a4e0e5a36fc997b7880eec45c0e ([bare_metal_support_2](https://github.com/TheCharlatan/bitcoin/tree/bare_metal_support_2) -> [bare_metal_support_3](https://github.com/TheCharlatan/bitcoin/tree/bare_metal_support_3), [compare](https://github.com/TheCharlatan/bitcoin/compare/bare_metal_support_2..bare_metal_support_3))
* Fixed conflict with #32989
🤔 maflcko reviewed a pull request: "ci: disable cirrus cache in 32bit arm job"
(https://github.com/bitcoin/bitcoin/pull/33302#pullrequestreview-3186038074)
also, looks like a cache outage in another task?
(https://github.com/bitcoin/bitcoin/pull/33302#pullrequestreview-3186038074)
also, looks like a cache outage in another task?
💬 instagibbs commented on issue "policy: allow RBF descendant carveout whenever conflicts exist, not just when number of conflicts == 1":
(https://github.com/bitcoin/bitcoin/issues/16819#issuecomment-3254477864)
I think we can just close this one; we're not going to improve carveouts at this point
(https://github.com/bitcoin/bitcoin/issues/16819#issuecomment-3254477864)
I think we can just close this one; we're not going to improve carveouts at this point
✅ fanquake closed an issue: "policy: allow RBF descendant carveout whenever conflicts exist, not just when number of conflicts == 1"
(https://github.com/bitcoin/bitcoin/issues/16819)
(https://github.com/bitcoin/bitcoin/issues/16819)
📝 fanquake opened a pull request: "doc: fix `LIBRARY_PATH` comment"
(https://github.com/bitcoin/bitcoin/pull/33308)
Now that we build capnp, qt isn't the only native package.
(https://github.com/bitcoin/bitcoin/pull/33308)
Now that we build capnp, qt isn't the only native package.
💬 katesalazar commented on pull request "gui: Avoid pathological QT text/markdown behavior...":
(https://github.com/bitcoin-core/gui/pull/886#issuecomment-3254546250)
> [...] I am not 100% sure what is causing this issue in QT's conversion of our HTML to markdown [...]
You implying that an upstream bug must be searched in Qt?
(https://github.com/bitcoin-core/gui/pull/886#issuecomment-3254546250)
> [...] I am not 100% sure what is causing this issue in QT's conversion of our HTML to markdown [...]
You implying that an upstream bug must be searched in Qt?
💬 katesalazar commented on pull request "gui: Avoid pathological QT text/markdown behavior...":
(https://github.com/bitcoin-core/gui/pull/886#issuecomment-3254557610)
Thank you
Approach ACK
possible nit:
```
PlainCopyQTextEdit
^
```
(https://github.com/bitcoin-core/gui/pull/886#issuecomment-3254557610)
Thank you
Approach ACK
possible nit:
```
PlainCopyQTextEdit
^
```
💬 fjahr commented on pull request "index: Fix coinstats overflow":
(https://github.com/bitcoin/bitcoin/pull/30469#issuecomment-3254558440)
Hm, not clear to me why clang-tidy CI complains here about lines I haven't touched in this PR. But that's what the last fixup commit I just pushed is for.
(https://github.com/bitcoin/bitcoin/pull/30469#issuecomment-3254558440)
Hm, not clear to me why clang-tidy CI complains here about lines I haven't touched in this PR. But that's what the last fixup commit I just pushed is for.
💬 katesalazar commented on pull request "gui: Avoid pathological QT text/markdown behavior...":
(https://github.com/bitcoin-core/gui/pull/886#issuecomment-3254577747)
nit: splash post in two paragraphs (none tiny) then the commit message is a one liner
(https://github.com/bitcoin-core/gui/pull/886#issuecomment-3254577747)
nit: splash post in two paragraphs (none tiny) then the commit message is a one liner
💬 glozow commented on issue "policy: allow RBF descendant carveout whenever conflicts exist, not just when number of conflicts == 1":
(https://github.com/bitcoin/bitcoin/issues/16819#issuecomment-3254599880)
Yeah the new design stages the changes and then checks policy limits. Currently in #28676: https://github.com/bitcoin/bitcoin/blob/61f51546af33923c9e5dc9632ba5544aa853ae71/src/validation.cpp#L1359-L1363
So the carveout becomes unnecessary since we aren't estimating cluster size, but calculating it
(https://github.com/bitcoin/bitcoin/issues/16819#issuecomment-3254599880)
Yeah the new design stages the changes and then checks policy limits. Currently in #28676: https://github.com/bitcoin/bitcoin/blob/61f51546af33923c9e5dc9632ba5544aa853ae71/src/validation.cpp#L1359-L1363
So the carveout becomes unnecessary since we aren't estimating cluster size, but calculating it