💬 Sjors commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#issuecomment-2476371060)
@TheCharlatan I adjusted the commit message

✅ Sjors closed a pull request: "Add waitFeesChanged() to Mining interface"
(https://github.com/bitcoin/bitcoin/pull/31003)

💬 Sjors commented on pull request "Add waitFeesChanged() to Mining interface":
(https://github.com/bitcoin/bitcoin/pull/31003#issuecomment-2476375818)
Actually I'm convinced the `waitNext()` approach is better.

💬 fanquake commented on pull request "guix: scope pkg-config to Linux only":
(https://github.com/bitcoin/bitcoin/pull/31276#issuecomment-2476385674)
> https://github.com/TheCharlatan/bitcoin/commit/1a6724af305122973efcc8a329e98f41077314a2

Pulled that in. Added another commit for sqlite.

💬 hodlinator commented on pull request "descriptor: Add proper Clone function to miniscript::Node":
(https://github.com/bitcoin/bitcoin/pull/30866#issuecomment-2476390623)
> Do you have a particular test case that triggered there?

Worked through several iterations of the `shared_ptr`-removal over a couple of days, at some point I had:
```C++
ret.emplace_back(std::make_unique<MiniscriptDescriptor>(std::move(pubs), std::move(*node)));
```
'cause I wasn't paying attention to the statement being inside a loop. The second descriptor created in the loop ended up with already-moved-from values, which resulted in an this test failing:

https://github.com/bitcoin/
...

💬 josibake commented on pull request "wallet, desc spkm: Return SigningProvider only if we have the privkey":
(https://github.com/bitcoin/bitcoin/pull/31242#issuecomment-2476403763)
Concept ACK

📝 Sjors opened a pull request: "Add destroy to BlockTemplate schema"
(https://github.com/bitcoin/bitcoin/pull/31288)
This ensures that if a client no longer needs a block template, the node can clear its memory as soon as possible.

A block template may hold on to transactions that are no longer in the mempool, so this can be significant.

✅ hodlinator closed a pull request: "test: Fix RANDOM_CTX_SEED use with parallel tests"
(https://github.com/bitcoin/bitcoin/pull/30737)

💬 hodlinator commented on pull request "test: Fix RANDOM_CTX_SEED use with parallel tests":
(https://github.com/bitcoin/bitcoin/pull/30737#issuecomment-2476412113)
Closing since the main issue was fixed in a different way by the now merged #31000.

💬 Sjors commented on pull request "Add destroy to BlockTemplate schema":
(https://github.com/bitcoin/bitcoin/pull/31288#issuecomment-2476412321)
@ryanofsky can you further clarify what the difference in behavior is with an without a `destroy` method?

👍 theStack approved a pull request: "wallet, desc spkm: Return SigningProvider only if we have the privkey"
(https://github.com/bitcoin/bitcoin/pull/31242#pullrequestreview-2436154466)
Concept and code-review ACK 493656763f73e5ef1cfb979a513c12983dca99dd

Convinced myself that this works as intended by writing a simple unit test: https://github.com/theStack/bitcoin/tree/pr31242_add_test
(could be added here or in a follow-up, though having to change the visibility of `GetSigningProvider` from private to public only for the sake of testing is a bit ugly...)

💬 instagibbs commented on pull request "policy: ephemeral dust followups":
(https://github.com/bitcoin/bitcoin/pull/31279#discussion_r1842272710)
we do in the following lines

👍 TheCharlatan approved a pull request: "guix: remove `util-linux`"
(https://github.com/bitcoin/bitcoin/pull/31285#pullrequestreview-2436171166)
ACK 4d668549825cc2f999b349e8fcd8cc9434c409c3

Guix build:
```
b681575760a0d19445a17d0d54f50a65d705923caee3a7cd7502e6611950dc11 guix-build-4d668549825c/output/aarch64-linux-gnu/SHA256SUMS.part
7af1420fedd8b4d10df650e92840287106dd74c289b6ef6fd2fd039387824647 guix-build-4d668549825c/output/aarch64-linux-gnu/bitcoin-4d668549825c-aarch64-linux-gnu-debug.tar.gz
bd6520732b5c7805002a3227fe2d16f56a77453d913f5204e5be869aaf4a9534 guix-build-4d668549825c/output/aarch64-linux-gnu/bitcoin-4d668549825
...

💬 instagibbs commented on pull request "policy: ephemeral dust followups":
(https://github.com/bitcoin/bitcoin/pull/31279#discussion_r1842274954)
It's a sentence fragment, I can touch it up to be grammatically correct

💬 instagibbs commented on pull request "Package validation: accept packages of size 1":
(https://github.com/bitcoin/bitcoin/pull/31096#discussion_r1842276747)
https://github.com/naumenkogs/bitcoin/commit/88c10c5789bfae1865d03966d1ce523e870ff0bc#diff-97c3a52bc5fad452d82670a7fd291800bae20c7bc35bb82686c2c0a4ea7b5b98R1811 weird comment UI, here is a direct link hopefully

🤔 l0rinc requested changes to a pull request: "validation: do not wipe utxo cache for stats/scans/snapshots"
(https://github.com/bitcoin/bitcoin/pull/30610#pullrequestreview-2435751891)
> It should not affect performance of the RPCs

It seems I have misunderstood multiple times what to benchmark here (IBD, reindex, RPC), so if there's anything that actually needs benchmarking, please describe it in more detail.

💬 l0rinc commented on pull request "validation: do not wipe utxo cache for stats/scans/snapshots":
(https://github.com/bitcoin/bitcoin/pull/30610#discussion_r1842018386)
It seems to me this complex condition contains a repeated hidden sub-expression that's needed later as `empty_cache`.
Could we split this up to reduce duplication and simplify the expressions?
```C++
bool should_empty_cache = (mode == FlushStateMode::FORCE_FLUSH) || fCacheLarge || fCacheCritical;
bool fDoFullFlush = should_empty_cache || (mode == FlushStateMode::FORCE_SYNC) || fPeriodicFlush || fFlushForPrune;
...
// Flush the chainstate (which may refer to block index entries).
if (shoul
...

💬 l0rinc commented on pull request "validation: do not wipe utxo cache for stats/scans/snapshots":
(https://github.com/bitcoin/bitcoin/pull/30610#discussion_r1842024022)
We should likely update the related script and docs as well:
* https://github.com/bitcoin/bitcoin/blob/master/contrib/tracing/log_utxocache_flush.py#L48
* https://github.com/bitcoin/bitcoin/blob/master/doc/tracing.md?plain=1#L130
* https://github.com/bitcoin/bitcoin/blob/master/contrib/tracing/README.md?plain=1#L251-L252

<details>
<summary>Patch</summary>

```diff
diff --git a/doc/tracing.md b/doc/tracing.md
--- a/doc/tracing.md (revision 1ede4803f1f5dffa55644d908062b52bf71394e7)
+++
...

💬 l0rinc commented on pull request "validation: do not wipe utxo cache for stats/scans/snapshots":
(https://github.com/bitcoin/bitcoin/pull/30610#discussion_r1842044510)
Same question here, is `Unconditionally` still the case here, given that `ForceFlushStateToDisk(false)` does a sync instead?

💬 l0rinc commented on pull request "validation: do not wipe utxo cache for stats/scans/snapshots":
(https://github.com/bitcoin/bitcoin/pull/30610#discussion_r1842041631)
Given that `ForceFlushStateToDisk(false)` does a `FORCE_SYNC` now, does the method name still reflect its role?