🚀 fanquake merged a pull request: "test: Simplify feature_fastprune.py"
(https://github.com/bitcoin/bitcoin/pull/27553)

💬 darosior commented on pull request "MiniTapscript: port Miniscript to Tapscript":
(https://github.com/bitcoin/bitcoin/pull/27255#discussion_r1183405185)
What's the rationale for changing this? Do whitespaces prevent the analysis? Happy to do this (and the other similar comments) but i'd rather not go through the hassle of applying it on every single of the 21 commits and solve the rebase conflicts if it's just a style nit. Let me know!

⚠️ Canvict opened an issue: "Bitcoin"
(https://github.com/bitcoin/bitcoin/issues/27560)

✅ fanquake closed an issue: "Bitcoin"
(https://github.com/bitcoin/bitcoin/issues/27560)

:lock: fanquake locked an issue: "Bitcoin"
(https://github.com/bitcoin/bitcoin/issues/27560)

💬 fanquake commented on pull request "build: include example bitcoin.conf in build outputs under share/examples/ subdirectory":
(https://github.com/bitcoin/bitcoin/pull/25935#issuecomment-1532665922)
Given the above, going to close this for now.

✅ fanquake closed a pull request: "build: include example bitcoin.conf in build outputs under share/examples/ subdirectory"
(https://github.com/bitcoin/bitcoin/pull/25935)

💬 darosior commented on pull request "MiniTapscript: port Miniscript to Tapscript":
(https://github.com/bitcoin/bitcoin/pull/27255#discussion_r1183410413)
This specific case is the very common match-all-variants-and-assert pattern so i think it's good like that. It's also documented this way in our style guidelines: https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#c-data-structures.

💬 darosior commented on pull request "MiniTapscript: port Miniscript to Tapscript":
(https://github.com/bitcoin/bitcoin/pull/27255#discussion_r1183419232)
For this and the other assertions of the context: if we get in such an inconsistent state as having a `multi_a` in a `wsh()` descriptor or a `multi` in a `tr()` descriptor, it's probably better to crash than to return inconsistent (and potentially harmful, like an unspendable address) data to the user. Though it could raise an exception instead of crashing the node. I'll see if i can use `CHECK_NONFATAL` instead.

💬 fanquake commented on pull request "net: call getaddrinfo() in detachable thread to prevent stalling":
(https://github.com/bitcoin/bitcoin/pull/27557#issuecomment-1532679609)
> The unit test I added makes a live DNS request for nic.com that test will fail the platform has no DNS

Or just anytime you don't have an internet connection? We're not going to add "internet connectivity" as a requirement to run the unit tests.

🚀 fanquake merged a pull request: "contrib: add ELF OS ABI check to symbol-check.py"
(https://github.com/bitcoin/bitcoin/pull/26953)

💬 darosior commented on pull request "MiniTapscript: port Miniscript to Tapscript":
(https://github.com/bitcoin/bitcoin/pull/27255#discussion_r1183444783)
Yes generally it would be nice to have better error reporting for descriptors. And i think there are lower hanging fruits when it comes to unclear descriptor errors that confuse users, for instance https://bitcoin.stackexchange.com/q/118022/101498.

But yeah definitely not going to bring that into this PR. :)

💬 darosior commented on pull request "MiniTapscript: port Miniscript to Tapscript":
(https://github.com/bitcoin/bitcoin/pull/27255#discussion_r1183446113)
Done.

💬 dergoegge commented on pull request "doc: clarify processing of mempool-msgs when NODE_BLOOM":
(https://github.com/bitcoin/bitcoin/pull/27559#issuecomment-1532728303)
ACK 4581a682d2d1fdd0e56fb4a56e6228be878a04a3

💬 josibake commented on pull request "Add fee_est tool for debugging fee estimation code":
(https://github.com/bitcoin/bitcoin/pull/10443#issuecomment-1532747667)
Thanks for the thorough response @ryanofsky. I realize I didn't explain myself very well in my initial comment.

> It sounds more like you're asking for an additional feature which would be easy to implement on top of the first commit, not pointing to a drawback of the approach

Since we already record mempool events in the debug log file (this is what @jamesob and I are using to log mempool events in [bmon](https://github.com/chaincodelabs/bmon)), it feels a bit strange to have special code
...

💬 josibake commented on pull request "bench: add readblock benchmark":
(https://github.com/bitcoin/bitcoin/pull/26684#issuecomment-1532763008)
Concept ACK

Based on the linked PRs, seems like having this benchmark is useful. There's also been some discussion about XOR'ing block data and having a benchmark for reading blocks would definitely be good to have before jumping into that.

I would, however, suggest holding off on merging this right now given that it conflicts with BIP324 PRs and is more of a nice to have vs something urgently needed.

💬 darosior commented on pull request "MiniTapscript: port Miniscript to Tapscript":
(https://github.com/bitcoin/bitcoin/pull/27255#discussion_r1183487461)
Thanks, good suggestion that i add those for clarity because your suggestion is incorrect: `add_diff` should always be added. The current (and intended) semantic is:
```diff
diff --git a/src/script/miniscript.h b/src/script/miniscript.h
index 011c7ac6d6..35797153da 100644
--- a/src/script/miniscript.h
+++ b/src/script/miniscript.h
@@ -1002,7 +1002,7 @@ private:
// this one?
next_sats_size.push_back((sats_size[j] + subs[i]->ss.dsat.size) |
...

💬 fanquake commented on issue "Followups from #19525":
(https://github.com/bitcoin/bitcoin/issues/19623#issuecomment-1532766941)
Going to close this for now.

✅ fanquake closed an issue: "Followups from #19525"
(https://github.com/bitcoin/bitcoin/issues/19623)

💬 willcl-ark commented on pull request "doc: clarify processing of mempool-msgs when NODE_BLOOM":
(https://github.com/bitcoin/bitcoin/pull/27559#issuecomment-1532773174)
ack 4581a682d2

💬 josibake commented on pull request "tests: Run both descriptor and legacy tests within a single test invocation":
(https://github.com/bitcoin/bitcoin/pull/20892#issuecomment-1532775504)
Concept ACK

I think when I initially looked at this PR the `BitcoinWalletTestFramework` class was breaking `TestShell`, but looking through the PR now, it seems the approach has changed to not use a `BitcoinWalletTestFramework` class. Can you update the description detailing the approach you are now taking?

Did a brief code review and the changes good look good, will test and review once you update the description so I can determine if the code is actually doing what you want it to do :smi
...