💬 l0rinc commented on pull request "test: Remove stale gettime test":
(https://github.com/bitcoin/bitcoin/pull/31846#issuecomment-2653744003)
ACK fa3a4eafa11ee03fccea1c2a16df5bf2ab164159

Your explanations make sense to me here.

💬 l0rinc commented on pull request "Benchmark Chainstate::ConnectBlock duration":
(https://github.com/bitcoin/bitcoin/pull/31689#discussion_r1952673009)
It's good general practice and it documents the code as well, e.g:
```
std::vector<CKey> keys{coinbaseKey};
keys.reserve(num_taproot + num_nontaproot + 1);
````

💬 fanquake commented on pull request "guix: Notarize MacOS app bundle and codesign all MacOS and Windows binaries":
(https://github.com/bitcoin/bitcoin/pull/31407#issuecomment-2653761199)
> Looks like everything matches:

I've run a build, but don't yet see everything matching:
```bash
find guix-build-096525e92cc2/output -wholename "*codesigned*" -type f -print0 | env LC_ALL=C sort -z | xargs -r0 sha256sum
207621cdc43868870f4136e9e6784a2a3e9ba89ec1edc6fa92b315cfa3c4432c guix-build-096525e92cc2/output/arm64-apple-darwin-codesigned/SHA256SUMS.part
111016205f0a2ac732feb934acb3e8a36d5251f119d8fa9215790310ba46c31d guix-build-096525e92cc2/output/arm64-apple-darwin-codesigned/bi
...

💬 Sjors commented on pull request "guix: Notarize MacOS app bundle and codesign all MacOS and Windows binaries":
(https://github.com/bitcoin/bitcoin/pull/31407#issuecomment-2653785671)
@pinheadmz yes that did the trick.

When running code sign I do get lots of warnings:

```
WARNING: Part of the file was not parsed: 4332 bytes
```

I get the same hashes as @fanquake (built on Ubuntu VM running on an M4 MacBook Pro in Qemu / UTM).

💬 willcl-ark commented on pull request "guix: Notarize MacOS app bundle and codesign all MacOS and Windows binaries":
(https://github.com/bitcoin/bitcoin/pull/31407#issuecomment-2653786937)
I have also finished a build, using pinheadmz signatures from https://github.com/pinheadmz/bitcoin-detached-sigs/tree/achow101-macos-notarization-096525e92cc2?rgh-link-date=2025-02-12T11%3A26%3A00Z

and get:

```bash
$ find guix-build-096525e92cc2/output -wholename "*codesigned*" -type f -print0 | env LC_ALL=C sort -z | xargs -r0 sha256sum
207621cdc43868870f4136e9e6784a2a3e9ba89ec1edc6fa92b315cfa3c4432c guix-build-096525e92cc2/output/arm64-apple-darwin-codesigned/SHA256SUMS.part
11101620
...

🚀 fanquake merged a pull request: "test: Remove stale gettime test"
(https://github.com/bitcoin/bitcoin/pull/31846)

💬 fanquake commented on pull request "[WIP] build: name install targets as components":
(https://github.com/bitcoin/bitcoin/pull/31847#issuecomment-2653826468)
Thank, although there is already a PR open to do this in #31844.

💬 Sjors 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_r1952737128)
That seems simpler anyway.

💬 maflcko commented on issue "`rpc_getblockstats.py` fails with `--gen-test-data`":
(https://github.com/bitcoin/bitcoin/issues/31838#issuecomment-2653847441)
The test should be using the miniwallet. Otherwise, it seems likely that some wallet change can affect the transactions and thus block stats.

💬 Sjors 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_r1952751154)
Oh, you mean `old_block` could be `nullopt` (assuming the RPC is loaded at this point).

But the suggested error here would change the current behavior - if there's a timeout we return the old block. I could do an early error return though if the first `getTip()` doesn't give us a tip.

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