🤔 sipa reviewed a pull request: "headerssync: Correct unrealistic unit test behavior"
(https://github.com/bitcoin/bitcoin/pull/32579#pullrequestreview-3202085490)
utACK 33d550d3044f9075cc866093c453158288f12dec

💬 sipa commented on pull request "headerssync: Correct unrealistic unit test behavior":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2333959308)
In commit "refactor(headerssync): Process spans of headers"

Nit: slightly simpler: `std::span{first_chain}.subspan(1)`.

💬 sipa commented on pull request "headerssync: Correct unrealistic unit test behavior":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2333960814)
In commit "refactor(headerssync): Process spans of headers"

Same here: `std::span{second_chain}.subspan(1)`.

💬 mzumsande commented on pull request "test: fix p2p_leak_tx.py":
(https://github.com/bitcoin/bitcoin/pull/33121#issuecomment-3271246212)
> My understanding of [`Peer::TxRelay::m_tx_inventory_known_filter`](https://github.com/bitcoin/bitcoin/blob/e04cb9c1bdf2199127cc8cf9c87f25e46b8cac7b/src/net_processing.cpp#L297-L300) is that it will [occasionally indicate](https://github.com/bitcoin/bitcoin/blob/e04cb9c1bdf2199127cc8cf9c87f25e46b8cac7b/src/common/bloom.h#L91-L92) that a transaction is known to a peer when it isn't, and then we won't include it in an INV:

Good point, I didn't think of that possibility.

> Maybe this is also
...

💬 ziggie1984 commented on issue "Difficulty in reliably mapping errors from Bitcoin Core due to unstable error codes and messages":
(https://github.com/bitcoin/bitcoin/issues/33350#issuecomment-3271254766)
Ok thank you for the information so far, feels like there is currently no easy way for us to proceed so we continue mapping the error strings. What would be very helpful is like a release-note highlight in case the error reporting for the RPC changes.

Keeping this issue open for a bit in case somebody else has a better idea, thank you so far for your thoughts.

💬 fanquake commented on pull request "guix: build for Linux HOSTS with `-static-libgcc`":
(https://github.com/bitcoin/bitcoin/pull/33181#issuecomment-3271268644)
@theuni switched this to using `CMAKE_EXE_LINKER_FLAGS`, which should make it clearer which flags are being used for executables.

💬 instagibbs commented on issue "Difficulty in reliably mapping errors from Bitcoin Core due to unstable error codes and messages":
(https://github.com/bitcoin/bitcoin/issues/33350#issuecomment-3271344694)
Chatting with some other devs, one other motivation for better/easier error codes would be to allow logging to be more sane.

For instance, `sendrawtransaction` may throw an error if already in mempool, which if logged is incredibly noisy and useless for fire-and-forget wallets like CLN.

submitpackage already behaves better in that respect, returning a success instead (as it is in the mempool).

💬 Sjors commented on pull request "wallet: warn against accidental unsafe older() import":
(https://github.com/bitcoin/bitcoin/pull/33135#issuecomment-3271397302)
Rebased. Fixed bug in the functional test found by an AI overlord, apparently using `is` isn't safe: https://stackoverflow.com/a/28864111

> I'll look into modifying miniscript / descriptor classes to avoid the string parsing.

Done, and also added a unit test. It's a bit more code and complexity, but more robust and extendable than parsing strings.

Since c1e6a9f1e7331fa2c1c73a0dd17056570283e443 is no longer relevant for this PR I dropped it.

💬 sipa commented on pull request "Bump SCRIPT_VERIFY flags to 64 bit":
(https://github.com/bitcoin/bitcoin/pull/32998#issuecomment-3271403052)
utACK 417437eb01ac014c57aca47f44d7f8d3da351987

💬 Sjors commented on pull request "index: Fix coinstats overflow":
(https://github.com/bitcoin/bitcoin/pull/30469#issuecomment-3271424867)
Oh oops...

```sh
bitcoin-cli gettxoutsetinfo muhash 913859
```

Both v29.1 and https://github.com/bitcoin/bitcoin/commit/c76797481155754329ec6a6f58e8402569043944:

```json
{
"height": 913859,
"bestblock": "00000000000000000000f7ae1989c423172433aab8aa0164f9fc85fd74ed7f44",
"txouts": 168005479,
"bogosize": 13163671088,
"muhash": "16bf45a4ce9c852aab746826955e3aac8e7e1b964a36af3bce944c17c5e13d55",
"total_amount": 19918082.42878334,
"total_unspendable_amount": 230.07121
...

💬 theuni commented on pull request "guix: build for Linux HOSTS with `-static-libgcc`":
(https://github.com/bitcoin/bitcoin/pull/33181#issuecomment-3271448380)
Switching to `CMAKE_EXE_LINKER_FLAGS` came out of an offline discussion with fanquake.

@fanquake we want all flags passed to all binaries with the exception of `-static-libstdc++ -static-libgcc`, which should only go to exe's.

So I think we want:
`HOST_LDFLAGS="-Wl,--as-needed -Wl,--dynamic-linker=$glibc_dynamic_linker -Wl,-O2"`
`CMAKE_EXE_LINKER_FLAGS="-static-libstdc++ -static-libgcc"`

Then for CMake, still pass `LDFLAGS` via `HOST_LDFLAGS` in addition to `CMAKE_EXE_LINKER_FLAGS`. T
...

💬 fanquake commented on pull request "guix: build for Linux HOSTS with `-static-libgcc`":
(https://github.com/bitcoin/bitcoin/pull/33181#issuecomment-3271470246)
Yea, I need to rework this in any case, as the macOS build breaks with the changes here.

👍 instagibbs approved a pull request: "Bump SCRIPT_VERIFY flags to 64 bit"
(https://github.com/bitcoin/bitcoin/pull/32998#pullrequestreview-3029931042)
ACK 417437eb01ac014c57aca47f44d7f8d3da351987

non-blocking suggestions for coverage

💬 instagibbs commented on pull request "Bump SCRIPT_VERIFY flags to 64 bit":
(https://github.com/bitcoin/bitcoin/pull/32998#discussion_r2213656693)
```Suggestion
BOOST_CHECK_EQUAL(FormatScriptFlags(SCRIPT_VERIFY_P2SH), "P2SH");
BOOST_CHECK_EQUAL(FormatScriptFlags(SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_TAPROOT), "P2SH,TAPROOT");
```

💬 instagibbs commented on pull request "Bump SCRIPT_VERIFY flags to 64 bit":
(https://github.com/bitcoin/bitcoin/pull/32998#discussion_r2213603725)
bit of coverage of multiple unknown bits (didn't validate)
```Suggestion
BOOST_CHECK_EQUAL(FormatScriptFlags(SCRIPT_VERIFY_TAPROOT | (1u<<27)), "TAPROOT,0x08000000");
BOOST_CHECK_EQUAL(FormatScriptFlags(SCRIPT_VERIFY_TAPROOT | (1u<<27) | (1u<<31)), "TAPROOT,0x88000000");
```

💬 instagibbs commented on pull request "Bump SCRIPT_VERIFY flags to 64 bit":
(https://github.com/bitcoin/bitcoin/pull/32998#discussion_r2334116135)
bddcadee82daf3ed1441820a0ffc4c5ef78f64f1

could be here or elsewhere, but round-simply round-tripping adds some coverage:

```
assert(script_verify_flags::from_int(flags.as_int()) == flags);
```

👋 Crypt-iQ's pull request is ready for review: "fuzz: compact block harness"
(https://github.com/bitcoin/bitcoin/pull/33300)

💬 Crypt-iQ commented on pull request "fuzz: compact block harness":
(https://github.com/bitcoin/bitcoin/pull/33300#issuecomment-3271483113)
There is non-determinism [here](https://github.com/bitcoin/bitcoin/blob/c0894a0a2be032cd9a5d5945643689230ab10255/src/node/blockstorage.cpp#L490-L493) because `m_dirty_blockindex` is `std::set<CBlockIndex*>` and sorts based on the pointer. This can be fixed by adding a function object that compares the block hashes. However, I did not know if this should be added just for fuzz code and I have not run benchmarks.

After patching the above non-determinism locally, there is also non-determinism in
...

⚠️ darosior opened an issue: "Higher **reported** memory usage of Bitcoin Core after version 29"
(https://github.com/bitcoin/bitcoin/issues/33351)
For versions of Bitcoin Core after 29.0, common utilities like `top` will report higher RAM usage of the `bitcoind` process than for previous Bitcoin Core versions.

The size of LevelDB files was increased from 2 MiB to 32 MiB in Bitcoin Core 29.0 (PR https://github.com/bitcoin/bitcoin/pull/30039). LevelDB caches its on-disk files using [`mmap`](https://en.wikipedia.org/wiki/Mmap) and will cache up to 1000 files by default, although our fork increased this number to 4096 (see https://github.com/
...

💬 BinaryIgor commented on pull request "rpc, logging: add backgroundvalidation to getblockchaininfo":
(https://github.com/bitcoin/bitcoin/pull/33259#discussion_r2334280301)
nit: typo, should be "validation"