💬 achow101 commented on pull request "wallet: Construct ScriptPubKeyMans with all data rather than loaded progressively":
(https://github.com/bitcoin/bitcoin/pull/28333#discussion_r2229646764)
Done

💬 achow101 commented on pull request "wallet: Construct ScriptPubKeyMans with all data rather than loaded progressively":
(https://github.com/bitcoin/bitcoin/pull/28333#discussion_r2229646853)
Done

💬 achow101 commented on pull request "wallet: Construct ScriptPubKeyMans with all data rather than loaded progressively":
(https://github.com/bitcoin/bitcoin/pull/28333#discussion_r2229646929)
Done

💬 achow101 commented on pull request "[IBD] prevector: store `P2WSH`/`P2TR`/`P2PK` scripts inline":
(https://github.com/bitcoin/bitcoin/pull/32279#issuecomment-3115094134)
ACK ec0b90e40e21ccb97c57373f7a7665ab9554acec

In a sample size of 1, this seems to provide a significant improvement in IBD time. It's surprising to me that it looks like this would have had benefit around the time that Taproot activated as previous measurements hadn't indicated that.

💬 jonatack commented on pull request "cli: return local services in -netinfo":
(https://github.com/bitcoin/bitcoin/pull/31886#discussion_r2229667232)
I've pulled in your suggestion here.

💬 jonatack commented on pull request "cli: return local services in -netinfo":
(https://github.com/bitcoin/bitcoin/pull/31886#discussion_r2229670245)
Not sure if it makes a measurable difference in performance, but I'm happy to use your version. Updated.

💬 jonatack commented on pull request "cli: return local services in -netinfo":
(https://github.com/bitcoin/bitcoin/pull/31886#issuecomment-3115124995)
Updated per review feedback (thanks!)

💬 achow101 commented on pull request "net: Fix Discover() not running when using -bind=0.0.0.0:port":
(https://github.com/bitcoin/bitcoin/pull/32757#issuecomment-3115210492)
Also seeing @mzumsande's observation if I add the addresses to loopback, but not if I add them to a different interface.

I think it's reasonable to filter out all loopback addresses, so the resolution is probably to just update the instructions to have the addresses bound to a non-loopback interface.

💬 monlovesmango commented on pull request "p2p: TxOrphanage revamp cleanups":
(https://github.com/bitcoin/bitcoin/pull/32941#issuecomment-3115217031)
My feedback is mostly a lot of annoying nits mainly from leftover variables and comments still referring to old max announcements DoS scoring. I compiled them in my own [branch](https://github.com/monlovesmango/bitcoin/tree/pr32941-TxOrphanage-revamp-cleanups) so you can just cherry pick whatever you think is actually worth taking. Overview below.

- [doc: Fixes GetChildrenFromSamePeer comments](https://github.com/monlovesmango/bitcoin/commit/2a25c28aec3773bdf9a17148e45ce38ee95a7342) - Updates
...

💬 pinheadmz commented on pull request "net: Fix Discover() not running when using -bind=0.0.0.0:port":
(https://github.com/bitcoin/bitcoin/pull/32757#issuecomment-3115236229)
If @b-l-u-e is going to retouch to update the instructions in the test comment I'd also really like to see those instructions executed in a CI test as well.

💬 mzumsande commented on pull request "net: Fix Discover() not running when using -bind=0.0.0.0:port":
(https://github.com/bitcoin/bitcoin/pull/32757#issuecomment-3115248007)
For me it would also ok to leave the resolution of #31336 for a separate PR - but this PR shouldn't claim to solve the issue then.

📝 murchandamus opened a pull request: "Slay BnB Mutants"
(https://github.com/bitcoin/bitcoin/pull/33060)
I tested all of the reported surviving mutants that @brunoerg reported in https://gist.github.com/brunoerg/834063398d5002f738506d741513e310.

I found that all Mutants except for 12, 14, 17, 37, and 39 were now being caught by one of the existing tests. This fixes Mutants 14, 37, and 39.

Mutant 17 is not fixed, because I consider it acceptable that running BnB for 100,001 instead of 100,000 comparisons doesn’t cause an issue, and Mutant 12 is not yet fixed, because at `fee` = `long_term_fee
...

💬 l0rinc commented on pull request "cli: return local services in -netinfo":
(https://github.com/bitcoin/bitcoin/pull/31886#discussion_r2229773767)
It's not necessarily about performance, rather just doing less work

💬 l0rinc commented on pull request "cli: return local services in -netinfo":
(https://github.com/bitcoin/bitcoin/pull/31886#discussion_r2229766850)
This should likely be an `std::string` instead

💬 l0rinc commented on pull request "cli: return local services in -netinfo":
(https://github.com/bitcoin/bitcoin/pull/31886#discussion_r2229779981)
nit: the log sounds a bit awkward

👍 theStack approved a pull request: "p2p: rename GetAddresses -> GetAddressesUnsafe"
(https://github.com/bitcoin/bitcoin/pull/32994#pullrequestreview-3053633002)
Code-review ACK 1cb23997033c395d9ecd7bf2f54787b134485f41

Nice documentation improvements, happy to see the method rename for improved clarity (fwiw, I'm generally not a huge fan of function overloading and think we tend to overuse it; it's often just confusing and makes it harder to find call-sites for a specific function, especially across multiple commits, e.g. for investigating past changes via `git log -S`).

📝 achow101 converted_to_draft a pull request: "Slay BnB Mutants"
(https://github.com/bitcoin/bitcoin/pull/33060)
I tested all of the reported surviving mutants that @brunoerg reported in https://gist.github.com/brunoerg/834063398d5002f738506d741513e310.

I found that all Mutants except for 12, 14, 17, 37, and 39 were now being caught by one of the existing tests. This fixes Mutants 14, 37, and 39.

Mutant 17 is not fixed, because I consider it acceptable that running BnB for 100,001 instead of 100,000 comparisons doesn’t cause an issue, and Mutant 12 is not yet fixed, because at `fee` = `long_term_fee
...

💬 w0xlt commented on pull request "Slay BnB Mutants":
(https://github.com/bitcoin/bitcoin/pull/33060#issuecomment-3115323386)
Concept ACK

💬 ismaelsadeeq commented on pull request "fees: rpc: `estimatesmartfee` now returns a fee rate estimate during low network activity":
(https://github.com/bitcoin/bitcoin/pull/32395#discussion_r2229962274)
Thanks for review.

> Why require anything at all here?

You check because you don't want to mislead the user. Imagine just finishing IBD or restarting a node after a long shutdown and then finishing the connection to the tip.
You don't want to instantly start reporting a 1 sat/vB fee rate. It has to ensure that there is indeed no activity in the network.

> What's the difference really between seeing 1 tx in the last n blocks and seeing none?

Seeing a transaction in the last n block
...

💬 Eunovo commented on pull request "Silent Payments: sending":
(https://github.com/bitcoin/bitcoin/pull/28201#discussion_r2230027031)
Makes sense