👍 wiz approved a pull request: "Testnet4 including PoW difficulty adjustment fix"
(https://github.com/bitcoin/bitcoin/pull/29775#pullrequestreview-2046012155)
Tested ACK @ 73c0cdc89bb3 from https://github.com/fjahr/bitcoin/commit/73c0cdc89bb3
Tested ACK @ cb895f48a743 from https://github.com/fjahr/bitcoin/tree/2024-04-testnet-4-fix-v27

Running cb895f48a743 on https://mempool.space/testnet4 using CPU mining, looks good to me

I noticed @Sjors seed at `seed.testnet4.bitcoin.sprovoost.nl` does not currently return any seeds, but I suppose that might simply be because his node doesn't know about any yet. I've added a bunch of my nodes at `seed.testn
...

🤔 edilmedeiros requested changes to a pull request: "test: Add a few more corner cases to the base58 test suite"
(https://github.com/bitcoin/bitcoin/pull/30035#pullrequestreview-2045711761)
Gave another deep look at this, thanks again for submitting this PR.

Beside the specific code comments, please add to the commit message why are you adding new test cases, what they improve in the test logic (and how they help the work on #29473).

💬 edilmedeiros commented on pull request "test: Add a few more corner cases to the base58 test suite":
(https://github.com/bitcoin/bitcoin/pull/30035#discussion_r1594064312)
```suggestion
BOOST_CHECK(DecodeBase58Check("3vQB7B6MrGQZaxCuFg4oh"s, result, 100));
```
Same as above.

💬 edilmedeiros commented on pull request "test: Add a few more corner cases to the base58 test suite":
(https://github.com/bitcoin/bitcoin/pull/30035#discussion_r1594060918)
Let's remove this since the messages now bring the test vectors themselves, this is duplicating purpose.

💬 edilmedeiros commented on pull request "test: Add a few more corner cases to the base58 test suite":
(https://github.com/bitcoin/bitcoin/pull/30035#discussion_r1594063834)
```suggestion
BOOST_CHECK(DecodeBase58("good"s, result, 100));
```
I understand the rational of aligning this with surrounding context, but does seem against guidelines and will look like a typo for others.

💬 edilmedeiros commented on pull request "test: Add a few more corner cases to the base58 test suite":
(https://github.com/bitcoin/bitcoin/pull/30035#discussion_r1594190131)
```suggestion
strTest << ": got \"" << EncodeBase58(sourcedata) << "\""
```
What about a little less verbose and taking advantage of the `strTest` string that has both input and expected outcome?

💬 edilmedeiros commented on pull request "test: Add a few more corner cases to the base58 test suite":
(https://github.com/bitcoin/bitcoin/pull/30035#discussion_r1594218804)
Don't need this since the test messages now bring the input vector.

💬 edilmedeiros commented on pull request "test: Add a few more corner cases to the base58 test suite":
(https://github.com/bitcoin/bitcoin/pull/30035#discussion_r1594206244)
```suggestion
BOOST_CHECK_MESSAGE(DecodeBase58(base58string, result, 256), strTest);
```
Testing for the same thing as before, but removing the test vector from the message don't seem to be an improvement. Potentially makes debugging more difficult.

💬 edilmedeiros commented on pull request "test: Add a few more corner cases to the base58 test suite":
(https://github.com/bitcoin/bitcoin/pull/30035#discussion_r1594203045)
```suggestion
strTest << ": got \"" << EncodeBase58(sourcedata) << "\""
```
Same suggestion as above.

💬 edilmedeiros commented on pull request "test: Add a few more corner cases to the base58 test suite":
(https://github.com/bitcoin/bitcoin/pull/30035#discussion_r1594213673)
I was in doubt this would check if the vectors contents are equal, but it does indeed. Nice catch.

💬 edilmedeiros commented on pull request "test: Add a few more corner cases to the base58 test suite":
(https://github.com/bitcoin/bitcoin/pull/30035#discussion_r1594240546)
Same thing about unreported random parameter in the test. This is worse yet since there's a weird logic to get the param. It's good that you submitted this PR, the original test deserved a better look already.

💬 edilmedeiros commented on pull request "test: Add a few more corner cases to the base58 test suite":
(https://github.com/bitcoin/bitcoin/pull/30035#discussion_r1594251265)
> Fixed, thanks! Added you as a Co-author, please check that the email is correct.

Not needed, but highly appreciated. Thanks.

💬 sdaftuar commented on pull request "[WIP] Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r1594255018)
Sjors suggested here https://github.com/bitcoin/bitcoin/pull/28676#discussion_r1399137900 that I add a way to do it by transaction hash, which would be easy. Let me know if you have other suggestions...

💬 sdaftuar commented on pull request "[WIP] Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r1594262607)
See line 528 below: https://github.com/sdaftuar/bitcoin/blob/c49e0444e5c474d5e1ac0af89e9bc27958f3ed31/src/kernel/txgraph.cpp#L528

💬 jadijadi commented on pull request "Keep focus on "Hide" while ModalOverlay is visible":
(https://github.com/bitcoin-core/gui/pull/795#issuecomment-2100893822)
Please do not hesitate to ask me if any other update is needed on this from my side. Thanks.

💬 sr-gi commented on pull request "net: Favor peers from addrman over fetching seednodes":
(https://github.com/bitcoin/bitcoin/pull/29605#discussion_r1594266520)
I haven't been able to reproduce the issue in my local setup, but I think we may be able to improve on this (plus reduce the waiting time) by mocking the node's time after restarting it

💬 sr-gi commented on pull request "net: Favor peers from addrman over fetching seednodes":
(https://github.com/bitcoin/bitcoin/pull/29605#discussion_r1594268166)
Tried that (plus some minimal improvements) in [138a2e2](https://github.com/bitcoin/bitcoin/pull/29605/commits/138a2e2a79846863fdce26102f800117f407ef97)

💬 ryanofsky commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1594227340)
In commit "rpc: identify JSON-RPC 2.0 requests" (6d0be75f3040c3190759c3ba6b5740897f2d3d29)

Might be good to add a comment noting that the "jsonrpc" version field was added in the JSON-RPC 2.0 spec, so value "1.0" is nonstandard. But it is accepted for backwards compatibility, because some old documentation (incorrectly) suggested adding it.

💬 ryanofsky commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1594229793)
In commit "rpc: identify JSON-RPC 2.0 requests" (6d0be75f3040c3190759c3ba6b5740897f2d3d29)

Variable name `valJsonRPC` doesn't really follow the suggested [coding style](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#coding-style-c) which calls for snake_case. Could rename it to "jsonrpc_version"

💬 ryanofsky commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1594231744)
In commit "rpc: identify JSON-RPC 2.0 requests" (6d0be75f3040c3190759c3ba6b5740897f2d3d29)

Technically JSON-RPC version would be more accurate the JSON version. could s/JSON/JSONRPC/ throughout this enum