💬 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
...

💬 darosior commented on pull request "MiniTapscript: port Miniscript to Tapscript":
(https://github.com/bitcoin/bitcoin/pull/27255#discussion_r1183502224)
I'm trying to reproduce the warning. What version of which compiler do you use? And what flags have you enabled?

📝 hebasto opened a pull request: "test: Explicitly specify directory where to search tests for"
(https://github.com/bitcoin/bitcoin/pull/27561)
For out-of-source builds, the `test/functional/test_runner.py` is supposed to be run from the build directory which allows it to pick the `test/config.ini` file generated by the build system. Currently, it works accidently for the following reasons:
- on POSIX systems, when running a created by Autoconf symlink to the `test/functional/test_runner.py` in the source directory, it actually has the source directory location in the `sys.path`.
- on Windows (the `build_msvc` directory) VS project pu
...

💬 hebasto commented on pull request "test: Explicitly specify directory where to search tests for":
(https://github.com/bitcoin/bitcoin/pull/27561#issuecomment-1532787387)
Friendly ping @stickies-v :)

💬 fanquake commented on pull request "doc: clarify processing of mempool-msgs when NODE_BLOOM":
(https://github.com/bitcoin/bitcoin/pull/27559#issuecomment-1532797807)
cc also @jnewbery @mzumsande @sdaftuar

💬 cefikhan commented on pull request "ScriptToUniv can get address of PUBKEY":
(https://github.com/bitcoin/bitcoin/pull/27546#issuecomment-1532800186)
> Concept NACK
>
> Sorry, no. P2PK outputs do not _have_ an address. It's an outdated and confusing practice to refer to them as their corresponding P2PKH address.

as far as my understanding

see we have
private key --> secp256k1 -- > public key --> sha256 --> RIPEMD160 --> Hashed Publickey --> hashed Publickey + checksum = wallet address

and we have the flow of ExtractDestination

PUBKEY -->passed to --> EXTRACT DESTINATION -->PKHash returned


PUBKEYHASH -->passed to --> EXTR
...

💬 dergoegge commented on pull request "net: call getaddrinfo() in detachable thread to prevent stalling":
(https://github.com/bitcoin/bitcoin/pull/27557#discussion_r1183531240)
Making actual requests from the unit tests is not great.

If you want to add a unit test, you could introduce a way to mock `getaddrinfo` or `AsyncGetAddrInfo` so that you can better test our code in all the relevant scenarios (i.e. getaddrinfo errors/blocking etc.).

💬 dergoegge commented on pull request "net: call getaddrinfo() in detachable thread to prevent stalling":
(https://github.com/bitcoin/bitcoin/pull/27557#discussion_r1183533168)
nit: Maybe use https://en.cppreference.com/w/cpp/thread/future/wait_until instead?