💬 ryanofsky commented on issue "test: 32-bit Clang `ipc_test` failure at `-O0`":
(https://github.com/bitcoin/bitcoin/issues/31772#issuecomment-2634113716)
Stack trace seems to be showing something going wrong in capnproto code, but unclear what the cause is. The crash is happening in the CidrRange::CidrRange constructor here:

https://github.com/capnproto/capnproto/blob/b34ec28cceaf15b1082b74b50f03f770873c3636/c%2B%2B/src/kj/cidr.c%2B%2B#L53

which is being called on a static list of address patterns:

https://github.com/capnproto/capnproto/blob/b34ec28cceaf15b1082b74b50f03f770873c3636/c%2B%2B/src/kj/async-io.c%2B%2B#L2999-L3007

All of the patter
...

💬 ryanofsky commented on issue "test: 32-bit Clang `ipc_test` failure at `-O0`":
(https://github.com/bitcoin/bitcoin/issues/31772#issuecomment-2634135091)
gdb shows invalid results being returned from `pattern.findFirst('/')` call where pattern is `192.0.0.0/24`

https://github.com/capnproto/capnproto/blob/b34ec28cceaf15b1082b74b50f03f770873c3636/c%2B%2B/src/kj/cidr.c%2B%2B#L51C48-L51C57

```
#14 0x5971f1e4 in kj::CidrRange::CidrRange (this=0x5a930c80 <kj::_::reservedCidrs()::result>, pattern=...) at /usr/src/kj/cidr.c++:53
(gdb) p pattern
$6 = {content = {<kj::DisallowConstCopyIfNotConst<char const>> = {<No data fields>}, ptr = 0x59e13a45 "192.0.
...

💬 willcl-ark commented on pull request "build: Enhance Ccache performance across worktrees and build trees":
(https://github.com/bitcoin/bitcoin/pull/30861#discussion_r1941284740)
We could also (or additionally) check if the user has set the launcher themselves by gating the entire following section with:

```cmake
if(NOT DEFINED CMAKE_CXX_COMPILER_LAUNCHER)
```

...skipping all ccache setup if they have.

This way we might be able to get the best of both "sane defaults" and "easy to override".

💬 hebasto commented on pull request "ci: build multiprocess on most jobs":
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2634149344)
> macOS cross compile fails (in a different way): https://cirrus-ci.com/task/5052270577975296
>
> ```
> [09:45:29.772] /bin/sh: 1: /ci_container_base/depends/x86_64-apple-darwin/bin/capnp: Exec format error
> ```

Here is a fix:
```diff
--- a/depends/toolchain.cmake.in
+++ b/depends/toolchain.cmake.in
@@ -155,6 +155,8 @@ endif()
if("@multiprocess@" STREQUAL "1")
set(ENABLE_IPC ON CACHE BOOL "")
set(Libmultiprocess_EXTERNAL_MPGEN "@depends_prefix@/native/bin/mpgen" CACHE PATH
...

💬 hebasto commented on pull request "ci: build multiprocess on most jobs":
(https://github.com/bitcoin/bitcoin/pull/30975#discussion_r1941299799)
Please, don't reintroduce more `@depends_prefix@` instances:
```suggestion
set(Libmultiprocess_EXTERNAL_MPGEN "${CMAKE_CURRENT_LIST_DIR}/native/bin/mpgen" CACHE FILEPATH "")
```

For the context, please see https://github.com/bitcoin/bitcoin/pull/31358.

💬 ajtowns commented on pull request "Double check all block rules in `ConnectBlock`, not only `CheckBlock`":
(https://github.com/bitcoin/bitcoin/pull/31792#discussion_r1941306874)
Should this line be in the following commit?

💬 danielabrozzoni commented on pull request "rpc: Ensure -debug=0/none behaves consistently with -nodebug":
(https://github.com/bitcoin/bitcoin/pull/31767#discussion_r1941344025)
You're right, thanks! Updated with "Logging" as it feels more appropriate to me too

💬 darosior commented on pull request "Double check all block rules in `ConnectBlock`, not only `CheckBlock`":
(https://github.com/bitcoin/bitcoin/pull/31792#discussion_r1941355885)
Done.

💬 Sjors commented on pull request "ci: build multiprocess on most jobs":
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2634256382)
Let's see if only ASan remains... https://github.com/bitcoin/bitcoin/actions/runs/13138410284/job/36659308174?pr=30975

💬 maflcko commented on pull request "test: check `scanning` field from `getwalletinfo`":
(https://github.com/bitcoin/bitcoin/pull/31768#issuecomment-2634283850)
lgtm ACK bb0879ddabc8b3a7253bc774d23b842937d18015

💬 maflcko commented on pull request "ci: run in worktrees":
(https://github.com/bitcoin/bitcoin/pull/31787#discussion_r1941383838)
On a second though, I guess if someone wanted to run the CI on an extracted tarball of the repo (git-archive), it would be better to also support that. Not sure if it is as trivial as `git init && git add ./`, so no strong opinion.

💬 darosior commented on pull request "Double check all block rules in `ConnectBlock`, not only `CheckBlock`":
(https://github.com/bitcoin/bitcoin/pull/31792#discussion_r1941384261)
To keep the existing level for this error. I suppose the existing level is debug due to log-filling concerns, maybe with very-low-difficulty headers?

Thanks for pointing this out though, i double checked the other log line in `TestBlockValidity` and there it should have been `LogError`. Fixed.

💬 ryanofsky commented on pull request "ci: build multiprocess on most jobs":
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2634296691)
re: https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2634256382

> Let's see if only ASan remains... https://github.com/bitcoin/bitcoin/actions/runs/13138410284/job/36659308174?pr=30975

Following might fix the asan error:

```diff
--- a/src/ipc/CMakeLists.txt
+++ b/src/ipc/CMakeLists.txt
@@ -9,6 +9,7 @@ if (NOT WITH_LIBMULTIPROCESS)
# Apply core_interface compile options to libmultiprocess runtime library.
target_link_libraries(multiprocess PUBLIC $<BUILD_INTERFACE:co
...

💬 ajtowns commented on pull request "Double check all block rules in `ConnectBlock`, not only `CheckBlock`":
(https://github.com/bitcoin/bitcoin/pull/31792#issuecomment-2634312015)
I think what you're doing here is:

* redoing the contextual checks in ConnectBlock
* currently, this could result in errors on startup if we accepted a block with timestamp T, but then our clock went backwards and is now set to a time prior to T-2 hours
* so to avoid that, we move the T+2h check out of the contextual checks into its own special function, and call that from the appropriate places
* once we redo contextual checks in ConnectBlock, `verifychain` rpc can detect contexual er
...

💬 darosior commented on pull request "Double check all block rules in `ConnectBlock`, not only `CheckBlock`":
(https://github.com/bitcoin/bitcoin/pull/31792#discussion_r1941394003)
No, it's to make this commit pass the test on its own. This is because this sub-test restart the node with a different deployment height for Segwit, and its caller then calls `verifychain`. This would fail if we don't restart the node with the default deployment height because `ConnectBlock` would detect unexpected witnesses before activation height. This is essentially what the next commit checks explicitly.

💬 Sjors commented on pull request "ci: build multiprocess on most jobs":
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2634356140)
I think that worked, and then found another issue:

```
/home/runner/work/_temp/src/ipc/libmultiprocess/src/mp/gen.cpp:633:26: error: comparison of integers of different signs: 'size_t' (aka 'unsigned long') and 'int' [-Werror,-Wsign-compare]
633 | for (size_t i = 4; i < argc; ++i) {
| ~ ^ ~~~~
1 error generated.
```

💬 theStack commented on pull request "crypto: Use secure_allocator for `AES256_ctx`":
(https://github.com/bitcoin/bitcoin/pull/31774#issuecomment-2634378908)
Concept ACK on clearing out the ctx/iv members

I'm wondering if a minimum-diff fix which simply replaces the `memset` calls in the dtors with `memory_cleanse` in the destructors would be largely sufficient here? In https://github.com/bitcoin/bitcoin/pull/31166#issuecomment-2446055905 one argument for not needing secure allocators was the short-lived nature of the secrets. Looking at the only usage in the wallet, this would imho apply here too (en/decrypting might take a while for larger input
...

📝 maflcko opened a pull request: "ci: Use clang-20 for sanitizer tasks"
(https://github.com/bitcoin/bitcoin/pull/31793)
A new clang version generally comes with bugfixes, new sanitizer features, deprecations, as well as new features.

Upgrade the sanitizer tasks to use the new version.

This was also suggested in https://github.com/bitcoin/bitcoin/pull/31691#issuecomment-2602517116

📝 furszy opened a pull request: "wallet: abandon orphan coinbase txs, and their descendants, during startup"
(https://github.com/bitcoin/bitcoin/pull/31794)
Since #26499, we mark coinbase transactions and their descendants as abandoned when a reorg arises through the "block disconnection" signal handler. However, this does not cover all scenarios; external wallets could contain coinbase transactions from blocks the node has not seen yet, or the user could have replaced the chain with an earlier or different version (one without the coinbase chain).

This affects balance calculation as well as descendants mempool rebroadcast (which shouldn't be rel
...

💬 sipa commented on pull request "crypto: Use secure_allocator for `AES256_ctx`":
(https://github.com/bitcoin/bitcoin/pull/31774#issuecomment-2634496045)
I think the approach here might have an unacceptable performance impact, as it is allocating and deallocating a secure area for every individual key being encrypted/decrypted.

It may be better to:
* Somehow make the `AES256CBC` classes members of `CCrypter`, surviving an individual encryption/decryption (e.g. by adding a reset function that can get called for each encryption/decryption, resetting the CBC state, but letting the key material survive).
* Follow @theStack's suggestion of not lo
...