💬 GarmashAlex commented on pull request "docs: sync external-signer.md with current external signer flow":
(https://github.com/bitcoin/bitcoin/pull/33947#issuecomment-3581066113)
> Thanks, however there is an existing PR at #33765. I'd suggest reviewing there first.
Oh, actually didn't notice that PR
(https://github.com/bitcoin/bitcoin/pull/33947#issuecomment-3581066113)
> Thanks, however there is an existing PR at #33765. I'd suggest reviewing there first.
Oh, actually didn't notice that PR
💬 naiyoma commented on pull request "test: refactor mempool_accept_wtxid":
(https://github.com/bitcoin/bitcoin/pull/33067#discussion_r2564803085)
thanks for the review, I’ve pushed an update and made the function more generic
(https://github.com/bitcoin/bitcoin/pull/33067#discussion_r2564803085)
thanks for the review, I’ve pushed an update and made the function more generic
💬 TheCharlatan commented on pull request "kernel: don't use assert to handle invalid user input":
(https://github.com/bitcoin/bitcoin/pull/33943#issuecomment-3581241991)
I'm Concept ACK on improving the behaviour around bad input, but not sure about adding more error handling surface to the API with the current approach. In my opinion the cases addressed are all programming errors, and should be treated as such. The argument that this duplicates the error handling code for the wrappers is not convincing to me and the PR itself as it currently stands provides the counter argument for it - an out of bounds case is still added and handled by the respective language
...
(https://github.com/bitcoin/bitcoin/pull/33943#issuecomment-3581241991)
I'm Concept ACK on improving the behaviour around bad input, but not sure about adding more error handling surface to the API with the current approach. In my opinion the cases addressed are all programming errors, and should be treated as such. The argument that this duplicates the error handling code for the wrappers is not convincing to me and the PR itself as it currently stands provides the counter argument for it - an out of bounds case is still added and handled by the respective language
...
🤔 janb84 reviewed a pull request: "depends: latest config.guess & config.sub"
(https://github.com/bitcoin/bitcoin/pull/33945#pullrequestreview-3510885486)
Post merge ACK 3e4355314b1abf8e4456ea41ba738aaae25abb73
I had a guix build still running, outcome is equal to the rest :)
my Guix Build Output
**Host architecture:** `aarch64`
**Commit:** `3e4355314b1a`
```shell
50d2ead905b87b4064fd66a0c0a2dc88a7ada65e2224f1ff992bc7ec685e493d guix-build-3e4355314b1a/output/aarch64-linux-gnu/SHA256SUMS.part
923b405f4eb8cb7158f2b4aef0918e48e7e57ef097871d0702dbbd5286013d32 guix-build-3e4355314b1a/output/aarch64-linux-gnu/bitcoin-3e4355314b1a-aa
...
(https://github.com/bitcoin/bitcoin/pull/33945#pullrequestreview-3510885486)
Post merge ACK 3e4355314b1abf8e4456ea41ba738aaae25abb73
I had a guix build still running, outcome is equal to the rest :)
my Guix Build Output
**Host architecture:** `aarch64`
**Commit:** `3e4355314b1a`
```shell
50d2ead905b87b4064fd66a0c0a2dc88a7ada65e2224f1ff992bc7ec685e493d guix-build-3e4355314b1a/output/aarch64-linux-gnu/SHA256SUMS.part
923b405f4eb8cb7158f2b4aef0918e48e7e57ef097871d0702dbbd5286013d32 guix-build-3e4355314b1a/output/aarch64-linux-gnu/bitcoin-3e4355314b1a-aa
...
🤔 rkrux reviewed a pull request: "Change Parse descriptor argument to string_view"
(https://github.com/bitcoin/bitcoin/pull/33914#pullrequestreview-3510903589)
lgtm ACK c0bfe72f6e1f63e05772eda959137b3d0bbbf6c3
(https://github.com/bitcoin/bitcoin/pull/33914#pullrequestreview-3510903589)
lgtm ACK c0bfe72f6e1f63e05772eda959137b3d0bbbf6c3
💬 laanwj commented on pull request "guix: build for Linux HOSTS with `-static-libgcc`":
(https://github.com/bitcoin/bitcoin/pull/33181#issuecomment-3581295709)
Sorry, we had a few delays and technical difficulties😓 (#33873). But good news, the riscv64 guix build just finally completed, and matches the other ones:
```
riscv64
05fc5cae41b19bbfdec5942e334e7b7d78ee0406ba69c221381b0b07b0a9e886 guix-build-f06c6e189831/output/aarch64-linux-gnu/SHA256SUMS.part
961613408c4d0b3b169488b43edc838135be0b712740b4015457f4f2808e103b guix-build-f06c6e189831/output/aarch64-linux-gnu/bitcoin-f06c6e189831-aarch64-linux-gnu-debug.tar.gz
906437c316d50e1f60fdfe2098fe7
...
(https://github.com/bitcoin/bitcoin/pull/33181#issuecomment-3581295709)
Sorry, we had a few delays and technical difficulties😓 (#33873). But good news, the riscv64 guix build just finally completed, and matches the other ones:
```
riscv64
05fc5cae41b19bbfdec5942e334e7b7d78ee0406ba69c221381b0b07b0a9e886 guix-build-f06c6e189831/output/aarch64-linux-gnu/SHA256SUMS.part
961613408c4d0b3b169488b43edc838135be0b712740b4015457f4f2808e103b guix-build-f06c6e189831/output/aarch64-linux-gnu/bitcoin-f06c6e189831-aarch64-linux-gnu-debug.tar.gz
906437c316d50e1f60fdfe2098fe7
...
💬 fanquake commented on pull request "guix: build for Linux HOSTS with `-static-libgcc`":
(https://github.com/bitcoin/bitcoin/pull/33181#issuecomment-3581307795)
@laanwj thanks for following up!
(https://github.com/bitcoin/bitcoin/pull/33181#issuecomment-3581307795)
@laanwj thanks for following up!
💬 instagibbs commented on pull request "test: Fix reorg patterns in tests to use proper fork-based approach":
(https://github.com/bitcoin/bitcoin/pull/32587#issuecomment-3581401865)
reACK 70d9e8f0a15d07a27ae37befb5c1bce71c98d8de
(https://github.com/bitcoin/bitcoin/pull/32587#issuecomment-3581401865)
reACK 70d9e8f0a15d07a27ae37befb5c1bce71c98d8de
💬 instagibbs commented on pull request "interfaces: remove redundant mempool lock in ChainImpl::isInMempool()":
(https://github.com/bitcoin/bitcoin/pull/33946#issuecomment-3581411365)
utACK 2909655fba91a7cc59c484fc74afafdf7ccc0cfa
(https://github.com/bitcoin/bitcoin/pull/33946#issuecomment-3581411365)
utACK 2909655fba91a7cc59c484fc74afafdf7ccc0cfa
📝 fanquake opened a pull request: "ubsan: add another suppression for InsecureRandomContext"
(https://github.com/bitcoin/bitcoin/pull/33949)
We've been suppressing this in infra for a little while, it's now appearing elsewhere. i.e
https://github.com/willcl-ark/bitcoin/actions/runs/19702968577/job/56443491265#step:9:2738:
```bash
Run bip324_cipher_roundtrip with args ['/home/runner/work/_temp/build/bin/fuzz', '-runs=1', PosixPath('/home/runner/work/_temp/ci/scratch/qa-assets/fuzz_corpora/bip324_cipher_roundtrip')]INFO: Running with entropic power schedule (0xFF, 100).
INFO: Seed: 2138761624
INFO: Loaded 1 modules (612416 inlin
...
(https://github.com/bitcoin/bitcoin/pull/33949)
We've been suppressing this in infra for a little while, it's now appearing elsewhere. i.e
https://github.com/willcl-ark/bitcoin/actions/runs/19702968577/job/56443491265#step:9:2738:
```bash
Run bip324_cipher_roundtrip with args ['/home/runner/work/_temp/build/bin/fuzz', '-runs=1', PosixPath('/home/runner/work/_temp/ci/scratch/qa-assets/fuzz_corpora/bip324_cipher_roundtrip')]INFO: Running with entropic power schedule (0xFF, 100).
INFO: Seed: 2138761624
INFO: Loaded 1 modules (612416 inlin
...
💬 fanquake commented on pull request "ubsan: add another suppression for InsecureRandomContext":
(https://github.com/bitcoin/bitcoin/pull/33949#issuecomment-3581424072)
cc @willcl-ark @dergoegge
(https://github.com/bitcoin/bitcoin/pull/33949#issuecomment-3581424072)
cc @willcl-ark @dergoegge
✅ fanquake closed an issue: "avoid using invalidateblock to directly test reorg behavior"
(https://github.com/bitcoin/bitcoin/issues/32531)
(https://github.com/bitcoin/bitcoin/issues/32531)
🚀 fanquake merged a pull request: "test: Fix reorg patterns in tests to use proper fork-based approach"
(https://github.com/bitcoin/bitcoin/pull/32587)
(https://github.com/bitcoin/bitcoin/pull/32587)
💬 sipa commented on pull request "Cluster mempool followups":
(https://github.com/bitcoin/bitcoin/pull/33591#discussion_r2565077473)
In commit "Simplify removeRecursive"
A possibility here, and below in `removeForReorg`, is to only add the child transactions themselves (not all their descendants) to a vector, in `TxGraph::Ref*` format, and then call `TxGraph::GetDescendantsUnion` once to find all their children to remove.
That would also make it acceptable, I think, to forego the epoch/`visited` check and just add everything, as `GetDescendantsUnion` will deduplicate anyway.
(https://github.com/bitcoin/bitcoin/pull/33591#discussion_r2565077473)
In commit "Simplify removeRecursive"
A possibility here, and below in `removeForReorg`, is to only add the child transactions themselves (not all their descendants) to a vector, in `TxGraph::Ref*` format, and then call `TxGraph::GetDescendantsUnion` once to find all their children to remove.
That would also make it acceptable, I think, to forego the epoch/`visited` check and just add everything, as `GetDescendantsUnion` will deduplicate anyway.
💬 sipa commented on pull request "Cluster mempool followups":
(https://github.com/bitcoin/bitcoin/pull/33591#discussion_r2565171834)
In commit "doc: add design notes for cluster mempool"
I think the theory thread is outdated now. Merging and postlinearization won't be very relevant post SFL, and the existence of optimal linearizations can probably be shown much more concisely when using results from the GGT paper.
It's probably worth creating a trimmed-down version with just definitions of clusters, chunks, and fee-size diagams, possibly to include directly in the document here, but that can be done as a follow-up I thi
...
(https://github.com/bitcoin/bitcoin/pull/33591#discussion_r2565171834)
In commit "doc: add design notes for cluster mempool"
I think the theory thread is outdated now. Merging and postlinearization won't be very relevant post SFL, and the existence of optimal linearizations can probably be shown much more concisely when using results from the GGT paper.
It's probably worth creating a trimmed-down version with just definitions of clusters, chunks, and fee-size diagams, possibly to include directly in the document here, but that can be done as a follow-up I thi
...
💬 sipa commented on pull request "Cluster mempool followups":
(https://github.com/bitcoin/bitcoin/pull/33591#discussion_r2565156011)
In commit "doc: Update mempool-limits.md for cluster mempool".
Perhaps worth elaborating that it is not just parents and children, but also any combination thereof:
> A transaction's *cluster* is the set of all transactions that can be reached by following any combination of parent or child relationships. This includes all ancestors and descendants, but also any descendants of those ancestors and ancestors of those descendants, and so on.
(https://github.com/bitcoin/bitcoin/pull/33591#discussion_r2565156011)
In commit "doc: Update mempool-limits.md for cluster mempool".
Perhaps worth elaborating that it is not just parents and children, but also any combination thereof:
> A transaction's *cluster* is the set of all transactions that can be reached by following any combination of parent or child relationships. This includes all ancestors and descendants, but also any descendants of those ancestors and ancestors of those descendants, and so on.
💬 hebasto commented on pull request "Clear out space on CentOS, depends, gui GHA job":
(https://github.com/bitcoin/bitcoin/pull/33514#issuecomment-3581524339)
@willcl-ark
Please rebase.
(https://github.com/bitcoin/bitcoin/pull/33514#issuecomment-3581524339)
@willcl-ark
Please rebase.
🤔 hebasto reviewed a pull request: "Clear out space on CentOS, depends, gui GHA job"
(https://github.com/bitcoin/bitcoin/pull/33514#pullrequestreview-3511188692)
Concept ACK. A similar approach was used in https://github.com/hebasto/bitcoin-core-nightly/pull/91.
(https://github.com/bitcoin/bitcoin/pull/33514#pullrequestreview-3511188692)
Concept ACK. A similar approach was used in https://github.com/hebasto/bitcoin-core-nightly/pull/91.
💬 willcl-ark commented on pull request "Clear out space on CentOS, depends, gui GHA job":
(https://github.com/bitcoin/bitcoin/pull/33514#issuecomment-3581540604)
> @willcl-ark
>
> Please rebase.
Sorry, was waiting to see if @maflcko 's background task approach works before adopting it here. I tested it in my fork [here](https://github.com/willcl-ark/bitcoin/actions/runs/19703413734/job/56444984809) where it appears to have worked successfully.
So I have pushed e07e57368e9fab8ecfc140d44aef7db9b23c7ce0 here which:
- applies the clearout to all jobs (on `gha` runners`)
- runs the task in the background
- takes the suggested command from https:
...
(https://github.com/bitcoin/bitcoin/pull/33514#issuecomment-3581540604)
> @willcl-ark
>
> Please rebase.
Sorry, was waiting to see if @maflcko 's background task approach works before adopting it here. I tested it in my fork [here](https://github.com/willcl-ark/bitcoin/actions/runs/19703413734/job/56444984809) where it appears to have worked successfully.
So I have pushed e07e57368e9fab8ecfc140d44aef7db9b23c7ce0 here which:
- applies the clearout to all jobs (on `gha` runners`)
- runs the task in the background
- takes the suggested command from https:
...
💬 willcl-ark commented on pull request "Clear out space on CentOS, depends, gui GHA job":
(https://github.com/bitcoin/bitcoin/pull/33514#discussion_r2565200327)
taken this suggestion in e07e57368e9fab8ecfc140d44aef7db9b23c7ce0. Thanks!
(https://github.com/bitcoin/bitcoin/pull/33514#discussion_r2565200327)
taken this suggestion in e07e57368e9fab8ecfc140d44aef7db9b23c7ce0. Thanks!