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

💬 josibake commented on pull request "rpc, p2p: allow `disconnectnode` with subnet":
(https://github.com/bitcoin/bitcoin/pull/26576#issuecomment-1532831791)
Concept ACK

I would recommend updating the description with a more clear use case, as I also wasn't convinced this was a good change until I read through the discussion and saw your comments @brunoerg

> This also seems out of scope for the disconnectnode rpc as it is meant (judging by the name and api right now) to disconnect a single node.

Counter argument: as mentioned in the description, `DisconnectNode` supports subnet as an argument:

https://github.com/bitcoin/bitcoin/blob/38d0
...

💬 jarolrod commented on pull request "qt: 25.0rc2 translations update":
(https://github.com/bitcoin/bitcoin/pull/27517#issuecomment-1532835443)
is this now pushed to rc3? cc @fanquake @achow101

💬 fanquake commented on pull request "qt: 25.0rc2 translations update":
(https://github.com/bitcoin/bitcoin/pull/27517#issuecomment-1532839992)
> is this now pushed to rc3?

An rc2 hasn't been tagged yet; not sure why it would be pushed to rc3.

💬 fanquake commented on issue "-Wmaybe-uninitialized warnings under LTO":
(https://github.com/bitcoin/bitcoin/issues/27343#issuecomment-1532841412)
Using master (49d543dcaf6ac1b71f8d607dab464a39aff837ac) & GCC 11.3.0 & LTO:
```bash
./univalue/include/univalue.h: In member function 'getInt':
./univalue/include/univalue.h:146:12: warning: 'result' may be used uninitialized in this function [-Wmaybe-uninitialized]
146 | return result;
| ^
./univalue/include/univalue.h:141:9: note: 'result' was declared here
141 | Int result;
| ^
./univalue/include/univalue.h: In member function 'getInt':
./u
...