💬 vasild commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1952751688)
Ok, I see, `-m|-M` is not mandatory and `bitcoin gui` has a reasonable default of running `bitcoin-qt` (as if `bitcoin -M gui` has been run. Makes sense, thanks for the explanation!

💬 fanquake commented on pull request "build: simplify by flattening the dependency graph":
(https://github.com/bitcoin/bitcoin/pull/30911#discussion_r1952761675)
No new movement here, but someone has offered another work around: https://gitlab.kitware.com/cmake/cmake/-/issues/24058#note_1624877

💬 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#issuecomment-2653871612)
> Previously it would return the last known tip during shutdown, but this creates an ambiguous circumstance in the scenario where the node is started and quickly shutdown, before notifications().TipBlock() is set.

New behavior seems good, but I am confused by this description. It looks to me like previous behavior was to segfault in this case, because it would return`Tip()->GetBlockHash()` and `Tip()` would be null? It would be helpful if description said more specifically what previous behav
...

🤔 sipa reviewed a pull request: "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs"
(https://github.com/bitcoin/bitcoin/pull/31829#pullrequestreview-2612094405)
Approach ACK

💬 sipa commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1952751403)
In commit "[txorphanage] when full, evict from the DoSiest peers first"

I think it would be helpful to add this variable, and the testing thereof, in a separate commit from the actual eviction changes.

💬 sipa commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1952760531)
Huh, neat, I hadn't considered using `FeeFrac` here, but it fits.

💬 sipa commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1952759514)
In commit "[txorphanage] when full, evict from the DoSiest peers first"

The `int64_t` return type won't actually do anything here, because `std::max` is instantiated for `unsigned int`, and also, the `std::map::size()` may return something smaller (particularly on 32-bit systems).

```c++
int64_t GetGlobalMaxUsage() const {
return std::max<int64_t>(int64_t(m_peer_orphanage_info.size()) * m_reserved_weight_per_peer, 1);
}
```

💬 sipa commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1952769273)
In commit "[txorphanage] when full, evict from the DoSiest peers first"

Using `FeeFrac::operator<` here, which tiebreaks by biggest denominator first in case the ratios are equal, means that if two peers are equally DoSsy, but one is that due to memory usage, and the other is that due to announcements, the announcements one will be targetted first. That's probably fine, but perhaps worth documenting.

💬 darosior commented on pull request "Double check all block rules in `ConnectBlock`, not only `CheckBlock`":
(https://github.com/bitcoin/bitcoin/pull/31792#issuecomment-2653904862)
Thanks, it's helpful to have numbers. I don't think a 3-4% IBD slowdown should necessarily be a deal breaker, but as discussed above i do not think this change is useful enough to be worth it.

💬 darosior commented on pull request "Double check all block rules in `ConnectBlock`, not only `CheckBlock`":
(https://github.com/bitcoin/bitcoin/pull/31792#issuecomment-2653926167)
> I'll try to bug `@`sdaftuar, who introduced this comment, to see if we are missing something.

Had a quick chat about this with Suhas and a couple others at Chaincode. My takeaway from the discussion is that if we wanted this we should probably go all the way to actually re-validate past connected blocks that were not fully validated, not only those received but not yet connected. Also there is no reason to see this comment as a blocker to introduce new consensus rules (depending on the natu
...

💬 0xB10C commented on pull request "tracing: network connection tracepoints":
(https://github.com/bitcoin/bitcoin/pull/25832#issuecomment-2653927636)
> Agree. Strings are sensitive to typos, and require extra effort for matching when doing statistics. i think even the current enumeration is better than that. BIP155 is a good suggestion, but as you have to make up non-standard values, and handle NET_UNROUTABLE differently i'm not entirely sure.
> The purpose of tracing is to gain insight into internal state so logging the actual values makes more sense than thinking up a new convention. The internal enumeration is likely stable enough to warr
...

💬 darosior commented on pull request "Double check all block rules in `ConnectBlock`, not only `CheckBlock`":
(https://github.com/bitcoin/bitcoin/pull/31792#issuecomment-2653935678)
I'll have a look at whether it's worth storing flags representing what rules have been checked against a block in the index, to enable re-validating past connected blocks against new rules when upgrading. In the meantime, this can be closed as i do not think it is worth doing on its own.

✅ darosior closed a pull request: "Double check all block rules in `ConnectBlock`, not only `CheckBlock`"
(https://github.com/bitcoin/bitcoin/pull/31792)

🤔 ryanofsky reviewed a pull request: "Have createNewBlock() wait for tip, make rpc handle shutdown during long poll and wait methods"
(https://github.com/bitcoin/bitcoin/pull/31785#pullrequestreview-2612158513)
Code review be15f2e769987b1df2cf1c8f327c5aafe064dbb3. Mostly looks good but I think vasild's suggestion https://github.com/bitcoin/bitcoin/pull/31785#discussion_r1952318566 should be used so waitTipChanged can't return an inconsistent hash and height

💬 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_r1952794396)
re: https://github.com/bitcoin/bitcoin/pull/31785#discussion_r1952303154

I don't think suggested text is accurate because if timeout is exceeded while waiting for a new tip, this returns information about the current tip. I don't think that behavior should be changed.

Returning null **only** when node is shutting down should be the simplest and best behavior.

💬 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_r1952788322)
In commit "rpc: handle shutdown during long poll and wait methods" (ad3af401c19b8d05ce69011a359db3090b7018e1)

Nice suggestion. If this suggestion is applied should also simplify the code above

```diff
- if (!tip_hash || chainman().m_interrupt) return {};
+ if (chainman().m_interrupt) return {};
```

since `tip_hash` is also relevant. Can also reduce the scope of the `tip_hash` variable.

💬 0xB10C commented on pull request "tracing: network connection tracepoints":
(https://github.com/bitcoin/bitcoin/pull/25832#discussion_r1952804176)
This should have been
```patch
- assert_equal(0, closed_connection.conn.network)
+ assert_equal(NETWORK_TYPE_UNROUTABLE, closed_connection.conn.network)
```

🚀 fanquake merged a pull request: "ci: Skip read-write of default env vars"
(https://github.com/bitcoin/bitcoin/pull/31678)

🚀 fanquake merged a pull request: "build: simplify by flattening the dependency graph"
(https://github.com/bitcoin/bitcoin/pull/30911)

💬 maflcko commented on pull request "ci: limit max stack size to 512 KiB":
(https://github.com/bitcoin/bitcoin/pull/31367#issuecomment-2653979844)
> It looks like `DEBUG=1` has an effect on this?

Maybe just enable it for the two GHA macOS tasks, which do not have DEBUG=1 enabled and also do not have sanitizers enabled?