💬 ryanofsky commented on pull request "Have createNewBlock() wait for tip, make rpc handle shutdown during long poll and wait methods":
(https://github.com/bitcoin/bitcoin/pull/31785#discussion_r2009145946)
In commit "Have createNewBlock() wait for a tip" (db14ca3556ca792546bf4343feb733271333690f)

Commit message is a little confusing because it doesn't mention the timeout change. Would be clearer if it said the commit was changing two things (1) returning null on shutdown instead of last tip (2) ignoring timeout value during startup instead of returning 0 if timeout elapsed before tip was connected

👍 ryanofsky approved a pull request: "rpc: add optional blockhash to waitfornewblock, unhide wait methods in help"
(https://github.com/bitcoin/bitcoin/pull/30635#pullrequestreview-2708626067)
Code review ACK e47b20f2f310f05832c879401660860cc40a6a09 with no changes since last review other than rebase

🤔 ryanofsky reviewed a pull request: "test: add missing segwitv1 test cases to `script_standard_tests`"
(https://github.com/bitcoin/bitcoin/pull/31340#pullrequestreview-2708628256)
@theStack this has two ACKs but also some unaddressed comments. It otherwise seems ok to merge since it is just a test change and small refactoring. Do you want to update the pr or respond to the comments?

💬 yancyribbens commented on issue "BnB untested/unused condition in UTXO exclusion optimization":
(https://github.com/bitcoin/bitcoin/issues/32047#issuecomment-2746287437)
> Yeah, I have had in my backlog that I would like to refactor BnB into the style of CoinGrinder since I wrote CoinGrinder, because I think it would be easier to understand, the iteration count would make more sense (currently it counts and evaluates the backtracking steps as well, but they can never yield a new solution!), and maybe easier to test. Given that my new job is explicitly more focused on working on the wallet, I was hoping to get to that once we are in the new office in April, but i
...

👍 ryanofsky approved a pull request: "CLI cleanups"
(https://github.com/bitcoin/bitcoin/pull/31887#pullrequestreview-2708629412)
Code review ACK d423fd9ec83ae323ac29bdc1b677ed8260bd59c4, just running clang-format and updating commit messages since last review

👍 ryanofsky approved a pull request: "Execute Discover() when bind=0.0.0.0 or :: is set"
(https://github.com/bitcoin/bitcoin/pull/31492#pullrequestreview-2708633985)
Code review ACK 1561575c2d984fd94c9679de3d1de207ef6c7c06, but would be nice to have feedback from @vasild here since this is supposed to fix #31293.

The changes here all seem to make sense but the interactions between them are complicated. I think this PR would be easier to review if the changes could be broken down as much as possible. It would be nice to add new tests in a first commit to clarify current buggy behaviors. Then it seems like the GetListenPort =onion parsing fix could be a sep
...

💬 ryanofsky commented on pull request "Execute Discover() when bind=0.0.0.0 or :: is set":
(https://github.com/bitcoin/bitcoin/pull/31492#discussion_r2009158096)
Might be missing something, but it seems like this could be simplified:

```c++
const CService onion_addr{DefaultOnionServiceTarget(GetListenPort() + 1);
```

🚀 ryanofsky merged a pull request: "CLI cleanups"
(https://github.com/bitcoin/bitcoin/pull/31887)

🤔 ryanofsky reviewed a pull request: "Fix tiebreak when loading blocks from disk (and add tests for comparing chain ties)"
(https://github.com/bitcoin/bitcoin/pull/29640#pullrequestreview-2708642977)
Might be ready for merge if @mzumsande and @sipa can re-ack

💬 darosior commented on issue "Enable PCP by default?":
(https://github.com/bitcoin/bitcoin/issues/31663#issuecomment-2746308933)
And if you do enable it does the port mapping works from `bitcoind`?

💬 Salarja commented on issue "depends: capnp build ignores config_opts":
(https://github.com/bitcoin/bitcoin/issues/32068#issuecomment-2746310181)
https://github.com/bitcoin/bitcoin/issues/32068

💬 yancyribbens commented on pull request "contrib: document asmap-tool commands more thoroughly":
(https://github.com/bitcoin/bitcoin/pull/32110#issuecomment-2746323958)
This was pretty interesting to read more about. It looks like ASmap can provide protection from certain types of threat actors, such as those that can allocate a large number of addresses from a single RIR effectively creating a type of Sybil attack. However, it seems to me there's another attack vector, such as a botnet that infects numerous systems across many RIR's that this wouldn't be effective against.

💬 sipa commented on pull request "contrib: document asmap-tool commands more thoroughly":
(https://github.com/bitcoin/bitcoin/pull/32110#discussion_r2009180426)
I don't think it defeats the purpose. It's just removing information one normally does not care about.

I'd just say "However, it does interfere with the ability to compute meaningful diffs".

💬 sipa commented on pull request "contrib: document asmap-tool commands more thoroughly":
(https://github.com/bitcoin/bitcoin/pull/32110#discussion_r2009179891)
Typo: `files are`, or `a file is`

💬 sipa commented on pull request "contrib: document asmap-tool commands more thoroughly":
(https://github.com/bitcoin/bitcoin/pull/32110#discussion_r2009180269)
Instead of accuracy, maybe say "This procedure is lossy because it loses information about which ranges were unassigned"? Arguably, no accuracy is lost, because all real IP addresses are still mapped to their correct AS.

💬 sipa commented on pull request "contrib: document asmap-tool commands more thoroughly":
(https://github.com/bitcoin/bitcoin/pull/32110#discussion_r2009180669)
Sure it can be, it's just less meaningful.

💬 sipa commented on pull request "contrib: document asmap-tool commands more thoroughly":
(https://github.com/bitcoin/bitcoin/pull/32110#discussion_r2009180746)
Same here.

💬 sipa commented on pull request "contrib: document asmap-tool commands more thoroughly":
(https://github.com/bitcoin/bitcoin/pull/32110#issuecomment-2746342015)
@yancyribbens

> However, it seems to me there's another attack vector, such as a botnet that infects numerous systems across many RIR's that this wouldn't be effective against.

Of course, but the theory is that getting access to many *reachable* IP addresses in many different network is far more expensive than in one or a few.

💬 yancyribbens commented on issue "Migrate from BTC/kvB to sat/vB on RPC and startup options":
(https://github.com/bitcoin/bitcoin/issues/32093#issuecomment-2746347346)
> I would recommend that we continue to use sat/kvB internally, maybe sat/kwu in the long-term, as we do only want to use integers but do want more precision than integer sat/vB

In rust-bitcoin land, the library uses sat/kwu as the default basis, and I think it would be rather nice if core and rust-bitcoin used the same default metric for FeeRate. So +1 for sat/kwu long-term idea.

💬 sipa commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r2009188901)
This works:

```c++
TxGraph::Ref ref;
...
ref = txgraph.AddTransaction(fee, size):
```

So think of an empty Ref as an equivalent to an `std::unique_ptr` to `nullptr` or so. It's potentially useful just so you can have a variable declared as a member in a class or so without immediately initializing it.

It even goes further in that this works even through subclasses:

```
class TxMempoolEntry : public TxGraph::Ref { ... }

TxMemoolEntry entry;
...
entry = txgraph.AddTransaction
...