💬 theuni commented on pull request "test, subprocess: Improve coverage report correctness":
(https://github.com/bitcoin/bitcoin/pull/30075#issuecomment-2103255846)
Are there some docs somewhere that suggest this?

💬 hebasto commented on pull request "test, subprocess: Improve coverage report correctness":
(https://github.com/bitcoin/bitcoin/pull/30075#issuecomment-2103262908)
> Are there some docs somewhere that suggest this?

https://gcc.gnu.org/onlinedocs/gcc/Gcov-and-Optimization.html

💬 theuni commented on issue "build: Unaligned libsecp256k1 flags in debug builds":
(https://github.com/bitcoin/bitcoin/issues/30055#issuecomment-2103276717)
I'm not sure this is a problem? It's hard to imagine a use-case where we'd want to be debugging Core with an un-optimized secp.

💬 achow101 commented on pull request "test: adds outbound eviction functional tests, updates comment in ConsiderEviction":
(https://github.com/bitcoin/bitcoin/pull/29122#issuecomment-2103348027)
ACK d53d84834747c37f4060a9ef379e0a6b50155246

👍 theuni approved a pull request: "kernel: Remove key module from kernel library"
(https://github.com/bitcoin/bitcoin/pull/29252#pullrequestreview-2048736866)
Very nice improvements since my last review. The RAII context is an obvious improvement, thanks @ryanofsky.

I looked for a way to add a virtual lifetime class with on_startup/on_shutdown to replace the .init in fuzz.h, but didn't have any luck because of a (afacs?) missing teardown callback. It seems the static context approach used here is actually what's recommended.

utACK 96378fe734e5fb6167eb20036d7170572a647edb

🚀 achow101 merged a pull request: "test: adds outbound eviction functional tests, updates comment in ConsiderEviction"
(https://github.com/bitcoin/bitcoin/pull/29122)

💬 achow101 commented on pull request "test: add MiniWallet tagging support to avoid UTXO mixing, use in `fill_mempool`":
(https://github.com/bitcoin/bitcoin/pull/29939#issuecomment-2103386014)
ACK dd8fa861939d5b8bdd844ad7cab015d08533a91a

👍 stickies-v approved a pull request: "[27.x] Backports"
(https://github.com/bitcoin/bitcoin/pull/29888#pullrequestreview-2048772665)
ACK 46890065e59875dd3d736d04f427e410bbd8faed

All backports clean except for 0fcceefe22532dc6389a95d2e058599e9496003b because of #28955, but the backport looks good to me

💬 stickies-v commented on pull request "[27.x] Backports":
(https://github.com/bitcoin/bitcoin/pull/29888#discussion_r1595946985)
I think we usually keep this header?

💬 stickies-v commented on pull request "[27.x] Backports":
(https://github.com/bitcoin/bitcoin/pull/29888#discussion_r1595950409)
```suggestion
- #29776 Fix #29767, set m_synced = true after Commit()
```

💬 stickies-v commented on pull request "[27.x] Backports":
(https://github.com/bitcoin/bitcoin/pull/29888#discussion_r1595965787)
Also, this is the commit message and not the PR title but this is a bit more descriptive so makes sense.

🚀 achow101 merged a pull request: "test: add MiniWallet tagging support to avoid UTXO mixing, use in `fill_mempool`"
(https://github.com/bitcoin/bitcoin/pull/29939)

💬 mzumsande commented on pull request "blockstorage: Separate reindexing from saving new blocks":
(https://github.com/bitcoin/bitcoin/pull/29975#discussion_r1596018428)
The current logic seems really brittle to me, I suspected that there might be a bug hiding somewhere, but it barely works out in all scenarios I could think of.
With blocks being save on disk out of order, it is possible that at the end of a reindex, the cursor points to an older block file. Yet, it appears that nothing really bad happens as a result: When a new blocks arrives, `FindBlockPos` will still find the correct position in [this while loop](https://github.com/bitcoin/bitcoin/blob/24572
...

💬 achow101 commented on pull request "test: use sleepy wait-for-log in reindex readonly":
(https://github.com/bitcoin/bitcoin/pull/30006#issuecomment-2103505565)
ACK fd6a7d3a13d89d74e161095b0e9bd3570210a40c

🚀 achow101 merged a pull request: "test: use sleepy wait-for-log in reindex readonly"
(https://github.com/bitcoin/bitcoin/pull/30006)

📝 theStack opened a pull request: "test: fix MiniWallet script-path spend (missing parity bit in leaf version)"
(https://github.com/bitcoin/bitcoin/pull/30076)
This PR fixes a dormant bug in MiniWallet that exists since support for P2TR was initially added in #23371 (see commit 041abfebe49ae5e3e882c00cc5caea1365a27a49).

In the course of spending the output, the leaf version byte of the control block in the witness stack doesn't set the parity bit, i.e. we were so far just lucky that the used combinations of relevant data (internal pubkey, leaf script / version) didn't result in a tweaked pubkey with odd y-parity. If that was the case, we'd get t
...

💬 tdb3 commented on pull request "RPC: Return `permitbaremultisig` and `maxdatacarriersize` in `getmempoolinfo`":
(https://github.com/bitcoin/bitcoin/pull/29954#issuecomment-2103628952)
> I wonder if a sub-object would be a good idea before we start adding more settings here. There's at least also `m_expiry` that would make sense to add too.

Implementing as a sub-object makes sense to me as well. If it is decided to adjust this RPC, then I'm thinking we'd also need a `-deprecatedrpc` option to retain the existing behavior, so we don't break RPC for downstream clients.

See also: https://github.com/bitcoin/bitcoin/pull/29845#discussion_r1571053657

💬 tdb3 commented on pull request "RPC: Return `permitbaremultisig` and `maxdatacarriersize` in `getmempoolinfo`":
(https://github.com/bitcoin/bitcoin/pull/29954#issuecomment-2103631437)
> Actually, since these don't change on their own... maybe a separate RPC altogether? (later extended to allow some changes?)

This option also makes sense to me. Maybe something like `getmempoolpolicy` or similar?

🤔 ajtowns reviewed a pull request: "test, subprocess: Improve coverage report correctness"
(https://github.com/bitcoin/bitcoin/pull/30075#pullrequestreview-2049083052)
Approach ACK

💬 ajtowns commented on pull request "test, subprocess: Improve coverage report correctness":
(https://github.com/bitcoin/bitcoin/pull/30075#discussion_r1596149065)
The code in https://inbox.sourceware.org/gcc-cvs/20200505141638.8B852388F43D@sourceware.org/T/ suggests that `__gcov_dump()` should be called before exec() because otherwise a successful exec will cause the information to be discarded, and `__gcov_reset()` should be called after exec() returns, because that means exec failed and the information wasn't discarded, so coverage stats should continue to be gathered without duplicating the info that's already been dumped. Doing the reset unconditional
...