💬 vasild commented on pull request "test: add end-to-end tests for CConnman and PeerManager":
(https://github.com/bitcoin/bitcoin/pull/26812#issuecomment-3244806630)
`bfe72353c0...5d7b70161d`: rebase due to conflicts
(https://github.com/bitcoin/bitcoin/pull/26812#issuecomment-3244806630)
`bfe72353c0...5d7b70161d`: rebase due to conflicts
💬 fanquake commented on pull request "kernel: chainparams & headersync updates for 30.0":
(https://github.com/bitcoin/bitcoin/pull/33274#discussion_r2315721583)
Bumped to 20.
(https://github.com/bitcoin/bitcoin/pull/33274#discussion_r2315721583)
Bumped to 20.
💬 fanquake commented on pull request "kernel: chainparams & headersync updates for 30.0":
(https://github.com/bitcoin/bitcoin/pull/33274#discussion_r2315721957)
> I think testnet4 blockchain size is bigger
Not here:
```bash
du -csh testnet4/chainstate/ testnet4/blocks/
945M testnet4/chainstate/
8.0G testnet4/blocks/
8.9G total
```
Will increase to 22 in any case.
(https://github.com/bitcoin/bitcoin/pull/33274#discussion_r2315721957)
> I think testnet4 blockchain size is bigger
Not here:
```bash
du -csh testnet4/chainstate/ testnet4/blocks/
945M testnet4/chainstate/
8.0G testnet4/blocks/
8.9G total
```
Will increase to 22 in any case.
💬 vasild commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2315723150)
Note that exception-inside-another-exception will still terminate the program, regardless of the added `noexcept(false)`.
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2315723150)
Note that exception-inside-another-exception will still terminate the program, regardless of the added `noexcept(false)`.
💬 ryanofsky commented on issue "intermittent timeout in mptest unit test":
(https://github.com/bitcoin/bitcoin/issues/33244#issuecomment-3244834811)
re: https://github.com/bitcoin/bitcoin/issues/33244#issuecomment-3243976122
> At the same time, it seems to happen frequently in CI, so I prefer my initial suggestion to either rewrite the failing unit test or remove it temporarily.
Sorry, I've been very distracted the past two weeks but I should be able to post a fix for this today.
It'd also be completely reasonable to disable the test, though I'm sure what a good way to disable it is because it's in a subtree.
(https://github.com/bitcoin/bitcoin/issues/33244#issuecomment-3244834811)
re: https://github.com/bitcoin/bitcoin/issues/33244#issuecomment-3243976122
> At the same time, it seems to happen frequently in CI, so I prefer my initial suggestion to either rewrite the failing unit test or remove it temporarily.
Sorry, I've been very distracted the past two weeks but I should be able to post a fix for this today.
It'd also be completely reasonable to disable the test, though I'm sure what a good way to disable it is because it's in a subtree.
💬 fanquake commented on pull request "kernel: chainparams & headersync updates for 30.0":
(https://github.com/bitcoin/bitcoin/pull/33274#discussion_r2315732101)
```bash
du -csh testnet3/chainstate/ testnet3/blocks/
16G testnet3/chainstate/
194G testnet3/blocks/
210G total
```
Can increase to 240.
(https://github.com/bitcoin/bitcoin/pull/33274#discussion_r2315732101)
```bash
du -csh testnet3/chainstate/ testnet3/blocks/
16G testnet3/chainstate/
194G testnet3/blocks/
210G total
```
Can increase to 240.
💬 fanquake commented on issue "ci: tidy job is producing output":
(https://github.com/bitcoin/bitcoin/issues/33256#issuecomment-3244852511)
> So if the output is a problem we could turn it off globally, or apply the patch there which suppresses it locally with a NOLINT annotation
I don't think we should have spurious/undocumented output in the CI, so it'd be good to fix this.
(https://github.com/bitcoin/bitcoin/issues/33256#issuecomment-3244852511)
> So if the output is a problem we could turn it off globally, or apply the patch there which suppresses it locally with a NOLINT annotation
I don't think we should have spurious/undocumented output in the CI, so it'd be good to fix this.
📝 stickies-v opened a pull request: "cmake: make missing Python interpreter behaviour more explicit"
(https://github.com/bitcoin/bitcoin/pull/33278)
We require Python 3.10 for multiple targets, and currently raise a general warning if it is missing, leading to:
1. functional test suite: runtime failure if python missing (as e.g. described in #31476)
2. maintenance targets: targets skipped if python missing
3. macos deploy target: target created, but build fails if python missing
This PR:
- adds a `BUILD_FUNCTIONAL_TESTS` option (default `BUILD_TESTS`, i.e. `ON`) and raises `FATAL_ERROR` if Python is missing and we're building the func
...
(https://github.com/bitcoin/bitcoin/pull/33278)
We require Python 3.10 for multiple targets, and currently raise a general warning if it is missing, leading to:
1. functional test suite: runtime failure if python missing (as e.g. described in #31476)
2. maintenance targets: targets skipped if python missing
3. macos deploy target: target created, but build fails if python missing
This PR:
- adds a `BUILD_FUNCTIONAL_TESTS` option (default `BUILD_TESTS`, i.e. `ON`) and raises `FATAL_ERROR` if Python is missing and we're building the func
...
💬 vasild commented on pull request "Fuzz: extend CConnman tests":
(https://github.com/bitcoin/bitcoin/pull/28584#discussion_r2315840032)
Good point! I do not know why I chose lowercase since it also violates the coding style. Maybe it was like that before and I left it as it is even though I touched those lines and the callers.
Append the following as a new commit or amend it into the last one `fuzz: make it possible to mock (fuzz) CThreadInterrupt`?
<details>
<summary>[patch] Uppercase methods of CThreadInterrupt</summary>
```diff
commit 486db14b9ce13a90b69f69dafb59d3d8932498f7 (HEAD -> fuzz_connman)
Parent: 0802398e
...
(https://github.com/bitcoin/bitcoin/pull/28584#discussion_r2315840032)
Good point! I do not know why I chose lowercase since it also violates the coding style. Maybe it was like that before and I left it as it is even though I touched those lines and the callers.
Append the following as a new commit or amend it into the last one `fuzz: make it possible to mock (fuzz) CThreadInterrupt`?
<details>
<summary>[patch] Uppercase methods of CThreadInterrupt</summary>
```diff
commit 486db14b9ce13a90b69f69dafb59d3d8932498f7 (HEAD -> fuzz_connman)
Parent: 0802398e
...
💬 vasild commented on pull request "Fuzz: extend CConnman tests":
(https://github.com/bitcoin/bitcoin/pull/28584#issuecomment-3244991538)
@brunoerg, this has 2 ACKs and a stale ACK from you, maybe you would like to take a look and re-ACK it?
(https://github.com/bitcoin/bitcoin/pull/28584#issuecomment-3244991538)
@brunoerg, this has 2 ACKs and a stale ACK from you, maybe you would like to take a look and re-ACK it?
⚠️ fanquake opened an issue: "build: clang-16 build broken on Debian Bookworm"
(https://github.com/bitcoin/bitcoin/issues/33279)
Our minimum required Clang is 16. Building with Clang-16 on Debian Bookworm is broken due to issue with capnp. See more details here: https://github.com/bitcoin-core/libmultiprocess/issues/199.
```bash
# clang-16 --version
Debian clang version 16.0.6 (15~deb12u1)
cmake --build build
...
In file included from /bitcoin/src/ipc/libmultiprocess/src/mp/util.cpp:6:
In file included from /bitcoin/src/ipc/libmultiprocess/include/mp/util.h:8:
In file included from /usr/include/capnp/schema.h:39:
In fil
...
(https://github.com/bitcoin/bitcoin/issues/33279)
Our minimum required Clang is 16. Building with Clang-16 on Debian Bookworm is broken due to issue with capnp. See more details here: https://github.com/bitcoin-core/libmultiprocess/issues/199.
```bash
# clang-16 --version
Debian clang version 16.0.6 (15~deb12u1)
cmake --build build
...
In file included from /bitcoin/src/ipc/libmultiprocess/src/mp/util.cpp:6:
In file included from /bitcoin/src/ipc/libmultiprocess/include/mp/util.h:8:
In file included from /usr/include/capnp/schema.h:39:
In fil
...
💬 stickies-v commented on pull request "build: Set AUTHOR_WARNING on warnings":
(https://github.com/bitcoin/bitcoin/pull/33144#issuecomment-3245006172)
> Sounds good.
Thanks for the feedback, I've opened #33278.
> Happy to put this in draft for now
The changes in this/your PR are small, straightforward, can easily be reverted, and offer immediate practical benefit, so I think it makes sense to not block this on my much less straightforward alternative, so my ACK still stands and I'd be happy if this makes progress.
(https://github.com/bitcoin/bitcoin/pull/33144#issuecomment-3245006172)
> Sounds good.
Thanks for the feedback, I've opened #33278.
> Happy to put this in draft for now
The changes in this/your PR are small, straightforward, can easily be reverted, and offer immediate practical benefit, so I think it makes sense to not block this on my much less straightforward alternative, so my ACK still stands and I'd be happy if this makes progress.
💬 Sjors commented on pull request "kernel: chainparams & headersync updates for 30.0":
(https://github.com/bitcoin/bitcoin/pull/33274#issuecomment-3245015482)
re-ACK 755152ac819a23acf2f9e70316134d74a04d589b
(https://github.com/bitcoin/bitcoin/pull/33274#issuecomment-3245015482)
re-ACK 755152ac819a23acf2f9e70316134d74a04d589b
👍 rkrux approved a pull request: "wallet: relax external_signer flag constraints"
(https://github.com/bitcoin/bitcoin/pull/33112#pullrequestreview-3176217358)
re-ACK 03978530ad8dc9124307d2ffc7d64c24b784be0e
```
git range-diff a973403...0397853
```
(https://github.com/bitcoin/bitcoin/pull/33112#pullrequestreview-3176217358)
re-ACK 03978530ad8dc9124307d2ffc7d64c24b784be0e
```
git range-diff a973403...0397853
```
⚠️ HowHsu opened an issue: "-loadblock indicated block files cannot find Magic Bytes"
(https://github.com/bitcoin/bitcoin/issues/33280)
### Is there an existing issue for this?
- [x] I have searched the existing issues
### Current behaviour
I first run `bitcoind -prune=1024 -stopatheight=840000 -datadir=empty_dir`, then there is some block files in `empty_dir/blocks`.
Then I copy empty_dir to empty_dir2, run `bitcoind -prune=1024 -stopatheight=840050 -datadir=empty_dir2`
Compare two blocks dir, I now have some blk.dat files relects height 840000 to 840050.
At last I run `bitcoind -datadir=empty_dir -assumevalid=0 -prune=10240
...
(https://github.com/bitcoin/bitcoin/issues/33280)
### Is there an existing issue for this?
- [x] I have searched the existing issues
### Current behaviour
I first run `bitcoind -prune=1024 -stopatheight=840000 -datadir=empty_dir`, then there is some block files in `empty_dir/blocks`.
Then I copy empty_dir to empty_dir2, run `bitcoind -prune=1024 -stopatheight=840050 -datadir=empty_dir2`
Compare two blocks dir, I now have some blk.dat files relects height 840000 to 840050.
At last I run `bitcoind -datadir=empty_dir -assumevalid=0 -prune=10240
...
💬 maflcko commented on pull request "Release: 30.0 translations update":
(https://github.com/bitcoin/bitcoin/pull/33275#issuecomment-3245095316)
Sure, see https://github.com/maflcko/b-c-gui-translations-review/tree/99bf41c4e5ab74eee8c248b240e8e940e47e09ec/reviews
(https://github.com/bitcoin/bitcoin/pull/33275#issuecomment-3245095316)
Sure, see https://github.com/maflcko/b-c-gui-translations-review/tree/99bf41c4e5ab74eee8c248b240e8e940e47e09ec/reviews
💬 Sjors commented on pull request "wallet/rpc: fix listdescriptors RPC fails to return descriptors with private key information when wallet contains descriptors missing any key":
(https://github.com/bitcoin/bitcoin/pull/32471#issuecomment-3245132183)
utACK 46860de809a1e9c7d0a1d716f77e9df7fb0bcd1c
I studied 4d07eff13002384ed59a3b7f592be56a25b88c15 from scratch since it had quite a few changes.
(https://github.com/bitcoin/bitcoin/pull/32471#issuecomment-3245132183)
utACK 46860de809a1e9c7d0a1d716f77e9df7fb0bcd1c
I studied 4d07eff13002384ed59a3b7f592be56a25b88c15 from scratch since it had quite a few changes.
💬 dergoegge commented on issue "asan: GCC warning about use-after-free":
(https://github.com/bitcoin/bitcoin/issues/33188#issuecomment-3245138164)
This is a false positive. If someone is using gcc with asan we could move `ASAN_UNPOISON_MEMORY_REGION(chunk, m_chunk_size_bytes);` above the `delete` call to avoid this warning (I suspect).
(https://github.com/bitcoin/bitcoin/issues/33188#issuecomment-3245138164)
This is a false positive. If someone is using gcc with asan we could move `ASAN_UNPOISON_MEMORY_REGION(chunk, m_chunk_size_bytes);` above the `delete` call to avoid this warning (I suspect).
💬 hebasto commented on pull request "Release: 30.0 translations update":
(https://github.com/bitcoin/bitcoin/pull/33275#issuecomment-3245221402)
> Sure, see https://github.com/maflcko/b-c-gui-translations-review/tree/99bf41c4e5ab74eee8c248b240e8e940e47e09ec/reviews
Friendly ping to coordinators for addressing issues:
- @sr-gi -- Catalan (ca)
- @ostruvek -- Czech (cs)
- @pryds -- Danish (da)
- @laanwj @sipa -- Dutch (nl)
- @Emzy -- German (de)
- @cryptomeow -- Greek (el)
- @jesterhodl -- Polish (pl)
(https://github.com/bitcoin/bitcoin/pull/33275#issuecomment-3245221402)
> Sure, see https://github.com/maflcko/b-c-gui-translations-review/tree/99bf41c4e5ab74eee8c248b240e8e940e47e09ec/reviews
Friendly ping to coordinators for addressing issues:
- @sr-gi -- Catalan (ca)
- @ostruvek -- Czech (cs)
- @pryds -- Danish (da)
- @laanwj @sipa -- Dutch (nl)
- @Emzy -- German (de)
- @cryptomeow -- Greek (el)
- @jesterhodl -- Polish (pl)
📝 ryanofsky opened a pull request: "clang-tidy: disable clang-analyzer UndefinedBinaryOperatorResult warning"
(https://github.com/bitcoin/bitcoin/pull/33281)
Disable `clang-analyzer-core.UndefinedBinaryOperatorResult` warning because it produces a false positive in a Cap'n Proto header:
```c++
/usr/include/kj/async-inl.h:609:37: warning: The left operand of '+' is a garbage value [clang-analyzer-core.UndefinedBinaryOperatorResult]
609 | return *(void**)(*(char**)obj + voff);
```
This was reported by fanquake in https://github.com/bitcoin/bitcoin/issues/33256 and was previously addressed in https://github.com/bitcoin-core/libmultiproc
...
(https://github.com/bitcoin/bitcoin/pull/33281)
Disable `clang-analyzer-core.UndefinedBinaryOperatorResult` warning because it produces a false positive in a Cap'n Proto header:
```c++
/usr/include/kj/async-inl.h:609:37: warning: The left operand of '+' is a garbage value [clang-analyzer-core.UndefinedBinaryOperatorResult]
609 | return *(void**)(*(char**)obj + voff);
```
This was reported by fanquake in https://github.com/bitcoin/bitcoin/issues/33256 and was previously addressed in https://github.com/bitcoin-core/libmultiproc
...