👍 hebasto approved a pull request: "ci: Checkout latest merged pulls"
(https://github.com/bitcoin/bitcoin/pull/33303#pullrequestreview-3186457692)
ACK fab00ebbc74fe55c41b2d531a964cdb083b02ab0.
This time, it took GitHub ~20min to update the reference.
(https://github.com/bitcoin/bitcoin/pull/33303#pullrequestreview-3186457692)
ACK fab00ebbc74fe55c41b2d531a964cdb083b02ab0.
This time, it took GitHub ~20min to update the reference.
💬 hebasto commented on pull request "ci, iwyu: Treat warnings as errors for specific directories":
(https://github.com/bitcoin/bitcoin/pull/31308#issuecomment-3254910524)
@ryanofsky
You may be interested in these changes.
(https://github.com/bitcoin/bitcoin/pull/31308#issuecomment-3254910524)
@ryanofsky
You may be interested in these changes.
💬 mzumsande commented on issue "natpmp: quiet down unconditional logging":
(https://github.com/bitcoin/bitcoin/issues/33301#issuecomment-3254930519)
> How about a exponential backoff, similar to how it's done in torcontrol.cpp?
That was just a random association, I don't know enough about PCP specifics to judge if it makes much sense.
Other questions would be:
- Does this situation deserve an unconditional warning at all instead of just logging to `net` - how actionable is the warning? (it seems to occur up for multiple people)
- Is it worth trying every 5 minutes forever instead of giving up after a number of tries? Maybe just log uncondi
...
(https://github.com/bitcoin/bitcoin/issues/33301#issuecomment-3254930519)
> How about a exponential backoff, similar to how it's done in torcontrol.cpp?
That was just a random association, I don't know enough about PCP specifics to judge if it makes much sense.
Other questions would be:
- Does this situation deserve an unconditional warning at all instead of just logging to `net` - how actionable is the warning? (it seems to occur up for multiple people)
- Is it worth trying every 5 minutes forever instead of giving up after a number of tries? Maybe just log uncondi
...
💬 fanquake commented on issue "natpmp: quiet down unconditional logging":
(https://github.com/bitcoin/bitcoin/issues/33301#issuecomment-3254938999)
cc @willcl-ark
(https://github.com/bitcoin/bitcoin/issues/33301#issuecomment-3254938999)
cc @willcl-ark
🤔 maflcko reviewed a pull request: "index: Fix coinstats overflow"
(https://github.com/bitcoin/bitcoin/pull/30469#pullrequestreview-3186533494)
> Hm, not clear to me why clang-tidy CI complains here about lines (or even files) I haven't touched in this PR. But that's what the last fixup commit I just pushed is for.
I haven't checked this, but the new struct is no longer trivially copyable, so it may have effects outside of the lines you have touched.
<details><summary>a diff</summary>
we should probably make it trivially copyable, but this seems unrelated.
```diff
diff --git a/src/arith_uint256.h b/src/arith_uint256.h
inde
...
(https://github.com/bitcoin/bitcoin/pull/30469#pullrequestreview-3186533494)
> Hm, not clear to me why clang-tidy CI complains here about lines (or even files) I haven't touched in this PR. But that's what the last fixup commit I just pushed is for.
I haven't checked this, but the new struct is no longer trivially copyable, so it may have effects outside of the lines you have touched.
<details><summary>a diff</summary>
we should probably make it trivially copyable, but this seems unrelated.
```diff
diff --git a/src/arith_uint256.h b/src/arith_uint256.h
inde
...
💬 maflcko commented on pull request "index: Fix coinstats overflow":
(https://github.com/bitcoin/bitcoin/pull/30469#discussion_r2323036958)
this looks like a false positive in clang-tidy and this looks like the wrong fix. `utxo_stats` is mutable, and taking a const reference does not make it immutable.
(In languages with a borrow checker, this wouldn't even compile, see https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=b8c78ddbe26b30473498b64a47ba2a6b)
The correct fix is to disable the clang-tidy rule in this line, or globally.
(https://github.com/bitcoin/bitcoin/pull/30469#discussion_r2323036958)
this looks like a false positive in clang-tidy and this looks like the wrong fix. `utxo_stats` is mutable, and taking a const reference does not make it immutable.
(In languages with a borrow checker, this wouldn't even compile, see https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=b8c78ddbe26b30473498b64a47ba2a6b)
The correct fix is to disable the clang-tidy rule in this line, or globally.
📝 glozow opened a pull request: "doc: archive v29.1 release notes"
(https://github.com/bitcoin/bitcoin/pull/33309)
(https://github.com/bitcoin/bitcoin/pull/33309)
💬 maflcko commented on pull request "ci: Checkout latest merged pulls":
(https://github.com/bitcoin/bitcoin/pull/33303#issuecomment-3254968070)
> Kinda in doubt about the use of YAML_KEY_01, in code it makes it clear that it is a YAML key but in the action execution it is visible as a `env` variable :
I know, but I don't know any alternative. See also the GitHub schema validator on a previous run: https://github.com/bitcoin/bitcoin/actions/runs/17471191651/workflow:
```
.github/workflows/ci.yml
Invalid workflow file
(Line: 21, Col: 1): Unexpected value 'checkout_ref_tmpl'
(https://github.com/bitcoin/bitcoin/pull/33303#issuecomment-3254968070)
> Kinda in doubt about the use of YAML_KEY_01, in code it makes it clear that it is a YAML key but in the action execution it is visible as a `env` variable :
I know, but I don't know any alternative. See also the GitHub schema validator on a previous run: https://github.com/bitcoin/bitcoin/actions/runs/17471191651/workflow:
```
.github/workflows/ci.yml
Invalid workflow file
(Line: 21, Col: 1): Unexpected value 'checkout_ref_tmpl'
💬 maflcko commented on pull request "Add functional test for IPC interface":
(https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2320936283)
nit: pip is now unused and can be removed
(https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2320936283)
nit: pip is now unused and can be removed
💬 maflcko commented on pull request "Add functional test for IPC interface":
(https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2323069799)
I don't think this is `-b`. You'll have to use `--revision` for raw commit ids? See https://git-scm.com/docs/git-clone#Documentation/git-clone.txt---revisionrev
(https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2323069799)
I don't think this is `-b`. You'll have to use `--revision` for raw commit ids? See https://git-scm.com/docs/git-clone#Documentation/git-clone.txt---revisionrev
💬 darosior commented on issue "natpmp: quiet down unconditional logging":
(https://github.com/bitcoin/bitcoin/issues/33301#issuecomment-3254998385)
cc @laanwj
(https://github.com/bitcoin/bitcoin/issues/33301#issuecomment-3254998385)
cc @laanwj
💬 hebasto commented on pull request "Release: 30.0 translations update":
(https://github.com/bitcoin/bitcoin/pull/33275#issuecomment-3254999297)
@jesterhodl
> > checkpoints
>
> Don't see it yet, I suppose it needs a sync? <img alt="image" width="587" height="187" src="https://private-user-images.githubusercontent.com/103882255/485108528-9efcb5a1-8b8c-4544-ab53-af574f2784c7.png?jwt=eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3NTcwMDk3NDksIm5iZiI6MTc1NzAwOTQ0OSwicGF0aCI6Ii8xMDM4ODIyNTUvNDg1MTA4NTI4LTllZmNiNWExLThiOGMtNDU0NC1hYjUzLWFmNTc0ZjI3O
...
(https://github.com/bitcoin/bitcoin/pull/33275#issuecomment-3254999297)
@jesterhodl
> > checkpoints
>
> Don't see it yet, I suppose it needs a sync? <img alt="image" width="587" height="187" src="https://private-user-images.githubusercontent.com/103882255/485108528-9efcb5a1-8b8c-4544-ab53-af574f2784c7.png?jwt=eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3NTcwMDk3NDksIm5iZiI6MTc1NzAwOTQ0OSwicGF0aCI6Ii8xMDM4ODIyNTUvNDg1MTA4NTI4LTllZmNiNWExLThiOGMtNDU0NC1hYjUzLWFmNTc0ZjI3O
...
🤔 hebasto reviewed a pull request: "depends: strip when installing qt binaries"
(https://github.com/bitcoin/bitcoin/pull/33304#pullrequestreview-3186600277)
My Guix build:
```
x86_64
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
997f4c68a
...
(https://github.com/bitcoin/bitcoin/pull/33304#pullrequestreview-3186600277)
My Guix build:
```
x86_64
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
997f4c68a
...
💬 janb84 commented on pull request "ci: Checkout latest merged pulls":
(https://github.com/bitcoin/bitcoin/pull/33303#issuecomment-3255010434)
> > Kinda in doubt about the use of YAML_KEY_01, in code it makes it clear that it is a YAML key but in the action execution it is visible as a `env` variable :
>
> I know, but I don't know any alternative. See also the GitHub schema validator on a previous run: https://github.com/bitcoin/bitcoin/actions/runs/17471191651/workflow:
>
> ```
> .github/workflows/ci.yml
>
> Invalid workflow file
>
> (Line: 21, Col: 1): Unexpected value 'checkout_ref_tmpl'
> ```
Isn't this because of
...
(https://github.com/bitcoin/bitcoin/pull/33303#issuecomment-3255010434)
> > Kinda in doubt about the use of YAML_KEY_01, in code it makes it clear that it is a YAML key but in the action execution it is visible as a `env` variable :
>
> I know, but I don't know any alternative. See also the GitHub schema validator on a previous run: https://github.com/bitcoin/bitcoin/actions/runs/17471191651/workflow:
>
> ```
> .github/workflows/ci.yml
>
> Invalid workflow file
>
> (Line: 21, Col: 1): Unexpected value 'checkout_ref_tmpl'
> ```
Isn't this because of
...
👍 hebasto approved a pull request: "depends: strip when installing qt binaries"
(https://github.com/bitcoin/bitcoin/pull/33304#pullrequestreview-3186608053)
ACK c9d5f211c119268d776af282dfd1e8b7590aaf56.
(https://github.com/bitcoin/bitcoin/pull/33304#pullrequestreview-3186608053)
ACK c9d5f211c119268d776af282dfd1e8b7590aaf56.
💬 romanz commented on pull request "index: store per-block transaction locations for efficient lookups":
(https://github.com/bitcoin/bitcoin/pull/32541#issuecomment-3255024309)
> Would be nice for the `LocationsIndex` if hashes were lazily computed (avoided) and transactions had a serialized size field that was cached upon deserialization so we don't need to call `GetSerializeSize`.
In https://github.com/romanz/bindex-rs, I am using https://github.com/RCasatta/bitcoin_slices which allows parsing Bitcoin blocks and transactions.
Maybe we can use a similar approach for building the indices (since each index may need a different part of the indexed transaction)?
(https://github.com/bitcoin/bitcoin/pull/32541#issuecomment-3255024309)
> Would be nice for the `LocationsIndex` if hashes were lazily computed (avoided) and transactions had a serialized size field that was cached upon deserialization so we don't need to call `GetSerializeSize`.
In https://github.com/romanz/bindex-rs, I am using https://github.com/RCasatta/bitcoin_slices which allows parsing Bitcoin blocks and transactions.
Maybe we can use a similar approach for building the indices (since each index may need a different part of the indexed transaction)?
💬 sipa commented on pull request "Add functional test for IPC interface":
(https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2323097757)
Fixed.
(https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2323097757)
Fixed.
💬 sipa commented on pull request "Add functional test for IPC interface":
(https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2323098780)
Thanks! It worked for a tag, but not for a commit.
(https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2323098780)
Thanks! It worked for a tag, but not for a commit.
💬 willcl-ark commented on pull request "build: suggest -DENABLE_IPC=OFF when missing capnp":
(https://github.com/bitcoin/bitcoin/pull/33290#discussion_r2323116369)
I have extended https://github.com/bitcoin-core/libmultiprocess/pull/205 to include more helpful error messages when the capnproto package is not found by cmake.
(https://github.com/bitcoin/bitcoin/pull/33290#discussion_r2323116369)
I have extended https://github.com/bitcoin-core/libmultiprocess/pull/205 to include more helpful error messages when the capnproto package is not found by cmake.
👍 l0rinc approved a pull request: "test: Remove polling loop from test_runner (take 2)"
(https://github.com/bitcoin/bitcoin/pull/33141#pullrequestreview-3186496990)
tested ACK fa4885ef2fde2b9fd9eb7367564a2946a45f20ac
(https://github.com/bitcoin/bitcoin/pull/33141#pullrequestreview-3186496990)
tested ACK fa4885ef2fde2b9fd9eb7367564a2946a45f20ac