💬 TheCharlatan commented on pull request "cmake: Do not modify `CMAKE_TRY_COMPILE_TARGET_TYPE` globally":
(https://github.com/bitcoin/bitcoin/pull/31662#discussion_r1961946736)
In commit 1b2ca134b9e94a26af0bcfc9d9142d0048db6089:

Nit (since this does get resolved eventually): Does removing the global in this commit actually have any effect if we still modify it in this macro? Seems confusing to say in the commit message that this commit removes the overrides when they are still there a few lines down.

💬 Sjors commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1961987717)
Fixed

💬 Sjors commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#issuecomment-2669140513)
Rebased and addressed feedback. I also reformatted the `BlockAssembler{` call to make it a bit less wide.

💬 0xB10C commented on pull request "ci: optionally use local docker build cache":
(https://github.com/bitcoin/bitcoin/pull/31545#discussion_r1961996102)
> However, running two mv at the same time seems like one should fail. And running mv and rm at the same time, seem like it could corrupt the cache.

If multiple jobs operate on the same build cache folder at the same time, then probably yes.

💬 dergoegge commented on pull request "cmake: Do not modify `CMAKE_TRY_COMPILE_TARGET_TYPE` globally":
(https://github.com/bitcoin/bitcoin/pull/31662#issuecomment-2669146601)
> s/SANITIZER_LDFLAGS/FUZZ_LIBS/

Happy to take care of this once this gets merged.

💬 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_r1962003488)
Looks like that indeed did the trick.

💬 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