💬 maflcko commented on pull request "ci: Switch to gcr.io mirror to avoid rate limits":
(https://github.com/bitcoin/bitcoin/pull/31906#issuecomment-2669160557)
> An alternative would probably be to configure the docker deamons of the self-hosted runners to use the mirror?

I haven't tested this and it seems like more hassle along with the risk of reintroducing the bug fixed in https://github.com/bitcoin/bitcoin/pull/28330. I guess the bug is fixed in the meantime via `CI_IMAGE_PLATFORM`, but I haven't tried it.

Though, I don't mind either way. I am happy to close this pull and review a different one, if someone creates one.

💬 Sjors commented on pull request "test, refactor: Add TestNode.binaries to hold binary paths":
(https://github.com/bitcoin/bitcoin/pull/31866#issuecomment-2669165993)
Concept ACK. Reviewed as part of #31375, but did not check if they're identical.

💬 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-2669172595)
> I've notarized the arm64 binaries, does running the downloaded the binaries still result in an error?

I'll try again on the new push, since my system might remember what I did to the previous binaries.

Do you want to push a linter fix, or should I just try the current version?

💬 davidgumberg commented on pull request "fuzz: a new target for the coins database":
(https://github.com/bitcoin/bitcoin/pull/28216#discussion_r1962021946)
This comment seems to me to document relevant and lasting context for what is otherwise a non-sequitur, anyone touching this line in the future should know what motivated putting it here.

🤔 pablomartin4btc reviewed a pull request: "test: fix TestShell initialization and reset()"
(https://github.com/bitcoin/bitcoin/pull/31415#pullrequestreview-2627399316)
tACK 318b2a2f90dfd8ae20beca58e55e6d240a7f3d27

<details>
<summary>I've managed to reproduce the issue in <code>master</code> and this PR fixes it.</summary>

- script:

```
cat shell.py
from pathlib import Path
import sys
REPO = Path(sys.argv.pop())
sys.path.insert(0, f"{REPO / 'build' / 'test' / 'functional'}")
from test_framework.test_shell import TestShell

TestShell().setup(num_nodes=1, setup_clean_chain=True)
TestShell().shutdown()
```


- `master` run:
```

python3
...

💬 achow101 commented on pull request "guix: Notarize MacOS app bundle and codesign all MacOS and Windows binaries":
(https://github.com/bitcoin/bitcoin/pull/31407#issuecomment-2669189571)
> Do you want to push a linter fix

Just pushed.

💬 glozow commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#issuecomment-2669192468)
Will add something like this as comments as well but here's the thinking around these benches:

- The `EraseForBlock` bench exists to test the worst case `EraseForBlock` time, which is every orphan conflicting the maximum amount with the block. That's (very roughly) ~2000 inputs per tx (within 400KWu), times the max number of orphans.
- We're kind of cheating in "limit EraseForBlock iterations and use set instead of vec" which forces it to stop at 100 orphans, even if the max number of orph
...

💬 pinheadmz commented on pull request "guix: Notarize MacOS app bundle and codesign all MacOS and Windows binaries":
(https://github.com/bitcoin/bitcoin/pull/31407#issuecomment-2669193479)
> I'll need new sigs from @pinheadmz anyway.
building e181bda stay tuned for sigs

💬 davidgumberg commented on pull request "build: Make config warnings fatal if -DWCONFIGURE_ERROR=ON":
(https://github.com/bitcoin/bitcoin/pull/31665#discussion_r1962032270)
Thanks, fixed

💬 davidgumberg commented on pull request "build: Make config warnings fatal if -DWCONFIGURE_ERROR=ON":
(https://github.com/bitcoin/bitcoin/pull/31665#issuecomment-2669204954)
Rebased to address reviewer feedback that the guix builds should have this option enabled, and that `CMakeLists.txt` uses 2 spaces for indentation, not 4.

💬 laanwj commented on pull request "Revert merge of PR #31826":
(https://github.com/bitcoin/bitcoin/pull/31908#issuecomment-2669210369)
> We shouldn't have merged https://github.com/bitcoin/bitcoin/pull/31826 if it didn't compile, and apparently nobody actually tested it. It's one thing to make changes for an unsupported platform that don't hurt others, but clearly the bar for testing was too low if it actually doesn't compile at all.

i did actually test it extensively, but yes it was changed and i hadn't gotten around to reviewing or re-testing it yet.

👍 laanwj approved a pull request: "Revert merge of PR #31826"
(https://github.com/bitcoin/bitcoin/pull/31908#pullrequestreview-2627438331)
ACK 3e9b12b3e0f039a8760410afed74c7e4d15afbe6
i have checked that this is an exact revert of 09b150bb8adae00854f02ece69fc6ef222fb07d9

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

Would be more consistent with other methods (and also clearer IMO) to just return `current_block` instead of `block` like:

```diff
--- a/src/rpc/blockchain.cpp
+++ b/src/rpc/blockchain.cpp
@@ -293,11 +293,11 @@ static RPCHelpMan waitfornewblock()
miner.waitTipChanged(current_block.hash);

// Return current block upon shutdo
...

👍 ryanofsky approved 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-2627395741)
Code review ACK b74b6c95eeba57ee6f627ceae0deeb6b14eadc0c

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

This change seems equivalent to previous code, and previous code seems clearer. This change also makes waitforblockheight implementation less consistent with waitforblock. Would suggest:

```diff
--- a/src/rpc/blockchain.cpp
+++ b/src/rpc/blockchain.cpp
@@ -406,8 +406,8 @@ static RPCHelpMan waitforblockheight()
std::optional<BlockRef> block;
if (timeout) {

...

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

This seem inconsistent with the commit message with says "a missing tip at startup now triggers NONFATAL_UNREACHABLE instead of CHECK_NONFATAL." Is commit message just out of date?

💬 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_r1962065125)
In commit "Have createNewBlock() wait for a tip" (a5b34064891814a998cac73c32f60d55a93e6811)

This is actually returning nullptr not nullopt

💬 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_r1962024170)
In commit "Handle negative timeout for waitTipChanged()" (f036e5091a90b75982684c5b64db1301d3ec2515)

IMO, new handling of negative timeouts seems reasonable to keep, but mentioning them in documentation is distracting. Would be less confusing to just shorten this to "(default is forever)".

💬 achow101 commented on pull request "Revert merge of PR #31826":
(https://github.com/bitcoin/bitcoin/pull/31908#issuecomment-2669284123)
> since this code is only targeting a not officially supported platform

Ostensibly we do though, as the code is part of the aarch64 platform which we do have CI for. However, it appears that our ARM CI does not have `HWCAP2_RNG`, but it could if we switched to using AWS Graviton instances.

💬 furszy commented on pull request "wallet: fix crash on double block disconnection":
(https://github.com/bitcoin/bitcoin/pull/31757#discussion_r1962092246)
Done as suggested