💬 m3dwards commented on pull request "net: Allow -proxy=[::1] on nodes with IPV6 lo only":
(https://github.com/bitcoin/bitcoin/pull/30245#issuecomment-2175762192)
Pushed new approach of calling `getaddrinfo` twice. First pass is unchanged from original behaviour, second pass adds local IP addresses should they have not been found on the first pass.
(https://github.com/bitcoin/bitcoin/pull/30245#issuecomment-2175762192)
Pushed new approach of calling `getaddrinfo` twice. First pass is unchanged from original behaviour, second pass adds local IP addresses should they have not been found on the first pass.
💬 m3dwards commented on pull request "ci: move ASan job to GitHub Actions from Cirrus CI":
(https://github.com/bitcoin/bitcoin/pull/30193#discussion_r1644231220)
Failing run: https://github.com/m3dwards/bitcoin/actions/runs/9354822480/job/25748533616
Looks ok to me, shows exit code 1.
For completeness, here is a run with a failing test with `CI_FAILFAST_TEST_LEAVE_DANGLING` removed: https://github.com/m3dwards/bitcoin/actions/runs/9562790625/job/26359909811. The job fails with exit code 137. The windows job behaves differently but I don't think that uses `--failfast`
(https://github.com/bitcoin/bitcoin/pull/30193#discussion_r1644231220)
Failing run: https://github.com/m3dwards/bitcoin/actions/runs/9354822480/job/25748533616
Looks ok to me, shows exit code 1.
For completeness, here is a run with a failing test with `CI_FAILFAST_TEST_LEAVE_DANGLING` removed: https://github.com/m3dwards/bitcoin/actions/runs/9562790625/job/26359909811. The job fails with exit code 137. The windows job behaves differently but I don't think that uses `--failfast`
💬 Sjors commented on pull request "Support self-hosted Cirrus workers on forks":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1644231641)
Changed to:
```cpp
# - There are no strict requirements on the hardware. Having fewer CPU threads
# than recommended merely causes the CI script to run slower.
```
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1644231641)
Changed to:
```cpp
# - There are no strict requirements on the hardware. Having fewer CPU threads
# than recommended merely causes the CI script to run slower.
```
💬 Sjors commented on pull request "Support self-hosted Cirrus workers on forks":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1644232676)
Oh, so only the maintainer of the fork will have this problem? Maybe there's another solution then.
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1644232676)
Oh, so only the maintainer of the fork will have this problem? Maybe there's another solution then.
💬 maflcko commented on pull request "Support self-hosted Cirrus workers on forks":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1644236071)
I think this extends to all hardware. For example using a spinning disk will also be slower, regardless of your CPU speed. So being less precise is more correct here.
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1644236071)
I think this extends to all hardware. For example using a spinning disk will also be slower, regardless of your CPU speed. So being less precise is more correct here.
💬 Sjors commented on pull request "Support self-hosted Cirrus workers on forks":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1644256566)
The list of machines only contain recommendations for CPUs and memory. Presumably giving them less memory than recommended can lead to OOM. Giving them fewer CPUs is the only recommendation you can safely ignore (if you're not in a CI environment that kills jobs after a timeout).
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1644256566)
The list of machines only contain recommendations for CPUs and memory. Presumably giving them less memory than recommended can lead to OOM. Giving them fewer CPUs is the only recommendation you can safely ignore (if you're not in a CI environment that kills jobs after a timeout).
💬 Sjors commented on pull request "Support self-hosted Cirrus workers on forks":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1644263459)
Ah wait, now I remember, reworded:
```
# When a contributor maintains a fork of the repo, any pull request they make
# to their own fork, or to the main repository, will trigger two CI runs:
# one for the branch push and one for the pull request.
# This can be avoided by setting NO_BRANCH=1 as a custom env variable in Cirrus.
```
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1644263459)
Ah wait, now I remember, reworded:
```
# When a contributor maintains a fork of the repo, any pull request they make
# to their own fork, or to the main repository, will trigger two CI runs:
# one for the branch push and one for the pull request.
# This can be avoided by setting NO_BRANCH=1 as a custom env variable in Cirrus.
```
💬 tdb3 commented on pull request "test: Validate oversized transactions or without inputs":
(https://github.com/bitcoin/bitcoin/pull/29862#discussion_r1644288241)
This is a good suggestion. If the file changes again, an explanation of "70" would add value to future reviewers.
(https://github.com/bitcoin/bitcoin/pull/29862#discussion_r1644288241)
This is a good suggestion. If the file changes again, an explanation of "70" would add value to future reviewers.
💬 tdb3 commented on pull request "test: Validate oversized transactions or without inputs":
(https://github.com/bitcoin/bitcoin/pull/29862#discussion_r1644326823)
Am I missing something simple? In https://github.com/bitcoin/bitcoin/pull/29862#discussion_r1643298389, it was mentioned that -70 should pass, but -69 should not. The test case appears to check that -70 fails with `bad-txns-oversize`. If -70 passes the size checking, it would fail for a reason other than `bad-txns-oversize`, which might be worth checking (i.e. shows that CheckTransaction is proceeding past the `bad-txns-oversize` check).
Disclaimer: I didn't check the txn byte math yet fo
...
(https://github.com/bitcoin/bitcoin/pull/29862#discussion_r1644326823)
Am I missing something simple? In https://github.com/bitcoin/bitcoin/pull/29862#discussion_r1643298389, it was mentioned that -70 should pass, but -69 should not. The test case appears to check that -70 fails with `bad-txns-oversize`. If -70 passes the size checking, it would fail for a reason other than `bad-txns-oversize`, which might be worth checking (i.e. shows that CheckTransaction is proceeding past the `bad-txns-oversize` check).
Disclaimer: I didn't check the txn byte math yet fo
...
🤔 tdb3 reviewed a pull request: "test: Validate oversized transactions or without inputs"
(https://github.com/bitcoin/bitcoin/pull/29862#pullrequestreview-2125235394)
Thanks.
Most of the updates look great. Left a question.
(https://github.com/bitcoin/bitcoin/pull/29862#pullrequestreview-2125235394)
Thanks.
Most of the updates look great. Left a question.
💬 pythcoiner commented on issue "docs: Wrong/outdated docs for `tr(KEY)` in doc/descriptors.md":
(https://github.com/bitcoin/bitcoin/issues/30279#issuecomment-2175909656)
> There it would also be great to expand the docs and link to https://bitcoin.sipa.be/miniscript/ (afaik there is no BIP for that).
There is re [recent PR](https://github.com/bitcoin/bips/pull/1610) in BIP repo.
(https://github.com/bitcoin/bitcoin/issues/30279#issuecomment-2175909656)
> There it would also be great to expand the docs and link to https://bitcoin.sipa.be/miniscript/ (afaik there is no BIP for that).
There is re [recent PR](https://github.com/bitcoin/bips/pull/1610) in BIP repo.
💬 sipa commented on issue "docs: Wrong/outdated docs for `tr(KEY)` in doc/descriptors.md":
(https://github.com/bitcoin/bitcoin/issues/30279#issuecomment-2175913472)
I agree with @benma. I think we should make the descriptors.md file more explicit, but also link to the relevant BIPs now.
(https://github.com/bitcoin/bitcoin/issues/30279#issuecomment-2175913472)
I agree with @benma. I think we should make the descriptors.md file more explicit, but also link to the relevant BIPs now.
👋 fanquake's pull request is ready for review: "build: add `-Wundef`"
(https://github.com/bitcoin/bitcoin/pull/29876)
(https://github.com/bitcoin/bitcoin/pull/29876)
💬 fanquake commented on pull request "build: add `-Wundef`":
(https://github.com/bitcoin/bitcoin/pull/29876#issuecomment-2175931399)
This is ready for further review.
(https://github.com/bitcoin/bitcoin/pull/29876#issuecomment-2175931399)
This is ready for further review.
💬 CharlesCNorton commented on pull request "fix: typo in benchmark documentation":
(https://github.com/bitcoin/bitcoin/pull/30296#issuecomment-2175943555)
> Thanks for your interest in contributing! However, as we have hundreds of pull requests, I am closing this to focus review on the others. (See [contributing guidelines on refactoring](https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#refactoring)).
Understood. I'll try to get a bigger collection of typos before pulling in the future.
(https://github.com/bitcoin/bitcoin/pull/30296#issuecomment-2175943555)
> Thanks for your interest in contributing! However, as we have hundreds of pull requests, I am closing this to focus review on the others. (See [contributing guidelines on refactoring](https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#refactoring)).
Understood. I'll try to get a bigger collection of typos before pulling in the future.
💬 fanquake commented on pull request "upnp: fix build with miniupnpc 2.2.8":
(https://github.com/bitcoin/bitcoin/pull/30283#issuecomment-2175946981)
Concept ACK - API fix without a bump seems fine.
> fanquake I see that you have been updating the bitcoin port in macports. Updating miniupnpc will break it. What do you think? Try to hold it until bitcoin-core v28?
If the miniupnpc package gets updated in macports, then pulling in the same patch that ends up being applied here (until it lands in a point release), for the bitcoin port seems like the best approach.
(https://github.com/bitcoin/bitcoin/pull/30283#issuecomment-2175946981)
Concept ACK - API fix without a bump seems fine.
> fanquake I see that you have been updating the bitcoin port in macports. Updating miniupnpc will break it. What do you think? Try to hold it until bitcoin-core v28?
If the miniupnpc package gets updated in macports, then pulling in the same patch that ends up being applied here (until it lands in a point release), for the bitcoin port seems like the best approach.
👋 glozow's pull request is ready for review: "doc: use TRUC instead of v3 and add release note"
(https://github.com/bitcoin/bitcoin/pull/30272)
(https://github.com/bitcoin/bitcoin/pull/30272)
💬 theuni commented on pull request "upnp: fix build with miniupnpc 2.2.8":
(https://github.com/bitcoin/bitcoin/pull/30283#issuecomment-2175976362)
Dropped the bump (will PR that separately) and rebased.
(https://github.com/bitcoin/bitcoin/pull/30283#issuecomment-2175976362)
Dropped the bump (will PR that separately) and rebased.
💬 fanquake commented on pull request "build: use `-no_exported_symbols` on macOS":
(https://github.com/bitcoin/bitcoin/pull/29072#issuecomment-2175984937)
Guix Build (aarch64):
```bash
45f5a3e38b5e2f8ef7f83f8b1e509d60cb933c52a0110dd6de6e43252869a62f guix-build-81d4dc8e8739/output/arm64-apple-darwin/SHA256SUMS.part
94e4578d894e61d6b96278a293e2c17fc19a8431bd77c996346fc7e126fc6b3e guix-build-81d4dc8e8739/output/arm64-apple-darwin/bitcoin-81d4dc8e8739-arm64-apple-darwin-unsigned.tar.gz
8f94554c9fdf356cbefd926944ece1ac39af26a80039d245e350b9194861af4a guix-build-81d4dc8e8739/output/arm64-apple-darwin/bitcoin-81d4dc8e8739-arm64-apple-darwin-unsign
...
(https://github.com/bitcoin/bitcoin/pull/29072#issuecomment-2175984937)
Guix Build (aarch64):
```bash
45f5a3e38b5e2f8ef7f83f8b1e509d60cb933c52a0110dd6de6e43252869a62f guix-build-81d4dc8e8739/output/arm64-apple-darwin/SHA256SUMS.part
94e4578d894e61d6b96278a293e2c17fc19a8431bd77c996346fc7e126fc6b3e guix-build-81d4dc8e8739/output/arm64-apple-darwin/bitcoin-81d4dc8e8739-arm64-apple-darwin-unsigned.tar.gz
8f94554c9fdf356cbefd926944ece1ac39af26a80039d245e350b9194861af4a guix-build-81d4dc8e8739/output/arm64-apple-darwin/bitcoin-81d4dc8e8739-arm64-apple-darwin-unsign
...
💬 fjahr commented on pull request "assumeutxo: Check snapshot base block is not in invalid chain":
(https://github.com/bitcoin/bitcoin/pull/30267#discussion_r1644425136)
I think these were always separated like that. But sounds good to me to move them. I have added a refactoring commit which moves the other two early checks into `ActivateSnapshot()` and then I am adding the new check there as well.
(https://github.com/bitcoin/bitcoin/pull/30267#discussion_r1644425136)
I think these were always separated like that. But sounds good to me to move them. I have added a refactoring commit which moves the other two early checks into `ActivateSnapshot()` and then I am adding the new check there as well.