💬 real-or-random commented on pull request "Add ASM optimizations for MuHash3072":
(https://github.com/bitcoin/bitcoin/pull/19181#issuecomment-1531563808)
> the SafeGCD change only impacts the finalize operation and that has a much smaller impact on the overall computation time because of how we use MuHash in practice. When a new block comes in we do a div of all the spent TXOs and a mul of all the new UTXOs and then at the end we finalize once.

Okay, thanks for the numbers. I agree then, we shouldn't neglect this one.

> So I still have a hard time deciding which one is the more valuable change. Also, while ASM might be a bit intimidating, h
...

💬 dergoegge commented on pull request "net: use interruptible async getaddrinfo wrapper from libevent for DNS":
(https://github.com/bitcoin/bitcoin/pull/27505#issuecomment-1531564374)
As an alternative to this PR, could we give the DNS thread time to join cleanly (e.g. 5s) and if it doesn't we just detach it and let the OS handle the clean up? That risks memory leaks but that shouldn't really matter when the program is about to exit anyway.

I would also be fine with not addressing this at all, because it seems like this only happens when a system generally fails to make DNS requests? In the absence of nice solutions, it seems like that isn't our problem and the user should
...

🤔 mzumsande reviewed a pull request: "Add support for "partial" fuzzers that indicate usefulness"
(https://github.com/bitcoin/bitcoin/pull/27552#pullrequestreview-1409209797)
> Maybe it's worth experimenting a bit with to so how much impact it has;

Yes, I'm planning to play with that, I'd be really interested in whether there is a significant speedup.

I feel like ideally, this would be something a good fuzzing engine should be able to handle to some extent without user guidance - uninteresting cases should create fewer additional seeds added to the corpus, which should result in them being picked by the engine for mutation less (and there might be more sophisti
...

👍 pinheadmz approved a pull request: "test: Simplify feature_fastprune.py"
(https://github.com/bitcoin/bitcoin/pull/27553#pullrequestreview-1409227264)
ACK fa17767154e21e9ed00782a9e4cf9a3d1d66f5d1

Thanks for simplifying, dunno why I didn't think of `generateblock` for this

<details><summary>Show Signature</summary>

```
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

ACK fa17767154e21e9ed00782a9e4cf9a3d1d66f5d1
-----BEGIN PGP SIGNATURE-----

iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmRRHVcACgkQ5+KYS2KJ
yTrfcA/+NVtObvE0dL9V50sLrQ6Qep0hJ9MTxdL/nGsa8fL3IOGnH/WV0npEytC3
FjvAOCbD84CHe2iIvzKkSkT93yDElB9Mz6c35TFbhwHFrRHSGCUK+IFWNu
...

💬 furszy commented on pull request "Introduce `MockableDatabase` for wallet unit tests":
(https://github.com/bitcoin/bitcoin/pull/26715#discussion_r1182632309)
Missing return statement.

📝 hebasto opened a pull request: "test: Treat `bitcoin-wallet` binary in the same way as others"
(https://github.com/bitcoin/bitcoin/pull/27554)
This PR makes the `bitcoin-wallet` binary path customizable in the same way how it can be done now with other ones, including `bitcoind`, `bitcoin-cli` and `bitcoin-util`.

💬 pinheadmz commented on pull request "net: use interruptible async getaddrinfo wrapper from libevent for DNS":
(https://github.com/bitcoin/bitcoin/pull/27505#issuecomment-1531618658)
@fanquake @dergoegge good feedback, thanks. I'll look into a different approach moving the lib call `getaddrinfo` to a detachable thread before abandoning, and then we may just have to close https://github.com/bitcoin/bitcoin/issues/16778 as "won't fix"

There is another thought I had about name resolution moving forward: If bitcoin can make more interesting DNS queries (either using a library or just implementing some bare necessities) we could ask the DNS seeders for I2P and onion addresses
...

💬 fanquake commented on pull request "net: use interruptible async getaddrinfo wrapper from libevent for DNS":
(https://github.com/bitcoin/bitcoin/pull/27505#issuecomment-1531621948)
> either using a library

It's very unlikely we are going to add a new external dependency to make "interesting" DNS queries.

💬 pinheadmz commented on pull request "blockstorage: do not flush block to disk if it is already there":
(https://github.com/bitcoin/bitcoin/pull/27039#issuecomment-1531627646)
> 🐙 This pull request conflicts with the target branch and [needs rebase](https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#rebasing-changes).

done ✅

💬 MarcoFalke commented on pull request "test: Treat `bitcoin-wallet` binary in the same way as others":
(https://github.com/bitcoin/bitcoin/pull/27554#discussion_r1182685633)
```suggestion
self.options.bitcoinwallet = os.getenv("BITCOINWALLET", default=fname("bitcoin-wallet"))
```

What about making this a function to reduce code bloat while touching this?

💬 ccdle12 commented on pull request "rpc: add 'getnetmsgstats', new rpc to view network message statistics":
(https://github.com/bitcoin/bitcoin/pull/27534#discussion_r1182690915)

nit: This constructor could use the default keyword since `byte_count` and `msg_count` will default to 0.

e.g. https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#c80-use-default-if-you-have-to-be-explicit-about-using-the-default-semantics

```
MsgStat() = default;
```

💬 instagibbs commented on issue "-fallbackfee should apply to estimatesmartfee":
(https://github.com/bitcoin/bitcoin/issues/27415#issuecomment-1531653495)
What's the problem with just doing error handling, and manually applying your own fallback fee? Faking a result is another layer of confusion imo

💬 ccdle12 commented on pull request "rpc: add 'getnetmsgstats', new rpc to view network message statistics":
(https://github.com/bitcoin/bitcoin/pull/27534#discussion_r1182691546)
According to this review comment: https://github.com/bitcoin/bitcoin/pull/27257#discussion_r1146305999 - It looks like it would be preferable for CNode to have net_stats as a member variable (I could be wrong) but that might mean each CNode would need to have a shared_ptr to a NetStats instance.

💬 ccdle12 commented on pull request "rpc: add 'getnetmsgstats', new rpc to view network message statistics":
(https://github.com/bitcoin/bitcoin/pull/27534#issuecomment-1531657509)
Concept tACK - left a few very minor comments, happy to keep testing and reviewing in more depth

⚠️ TheBlueMatt opened an issue: "Avoid serving stale fees"
(https://github.com/bitcoin/bitcoin/issues/27555)
After conversation on irc it appears there's some cases where Bitcoin core will serve feerate information even though it loaded a fairly old fee estimates file and is still syncing the chain. This is pretty dangerous for lightning nodes, at least on legacy channels or pre-package-relay.

The immediate improvement would be to store fee estimates to disk once an hour or so to reduce the chance of having an old file. From there this case could probably be detected and refuse to serve estimates unti
...

💬 achow101 commented on pull request "Introduce `MockableDatabase` for wallet unit tests":
(https://github.com/bitcoin/bitcoin/pull/26715#discussion_r1182709725)
Oops, fixed

💬 instagibbs commented on pull request "mempool: keep CPFP'd transactions when loading from mempool.dat":
(https://github.com/bitcoin/bitcoin/pull/27476#issuecomment-1531678984)
Special-casing it is a value-add imo: v3/ephemeral anchor tx should be aggressively fee bumping anyways.

💬 kylezs commented on issue "-fallbackfee should apply to estimatesmartfee":
(https://github.com/bitcoin/bitcoin/issues/27415#issuecomment-1531690323)
> What's the problem with just doing error handling, and manually applying your own fallback fee? Faking a result is another layer of confusion imo

Because a -fallback fee already exists. Why can't it exist for other use cases?

💬 achow101 commented on pull request "rpc: Return block hash & height in getbalances, gettransaction and getwalletinfo":
(https://github.com/bitcoin/bitcoin/pull/26094#issuecomment-1531700243)
ACK 710b83938ab5bbc4bd324d8b2e69461a2a1d2eec

💬 instagibbs commented on issue "-fallbackfee should apply to estimatesmartfee":
(https://github.com/bitcoin/bitcoin/issues/27415#issuecomment-1531711091)
If you absolutely cannot handle RPC exceptions your test harness could also:

1) mock out estimatesmartfee calls to give something deterministic
2) load a saved fee estimation file