📝 vasild opened a pull request: "net: log connections failures via SOCKS5 with less severity"
(https://github.com/bitcoin/bitcoin/pull/30064)
It is expected to have some Bitcoin nodes unreachable some of the time. A failure to connect to an IPv4 or IPv6 node is already properly logged under category=net/severity=debug. Do the same when a connection fails when using a SOCKS5 proxy. This could be either to an .onion address or to an IPv4 or IPv6 address (via a Tor exit node).

Related: https://github.com/bitcoin/bitcoin/issues/29759

💬 vasild commented on issue "Tons of Socks5() connect to x.x.x.x:8333 failed: connection refused-messages when i use TOR - why?":
(https://github.com/bitcoin/bitcoin/issues/29759#issuecomment-2100763683)
> There is an inconsistency wrt logging connection failures...

Addressed in https://github.com/bitcoin/bitcoin/pull/30064

💬 ryanofsky commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1594138363)
re: https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1593964758

I think @cbergqvist is right here and `e` should be replaced by `std::move(e)` on line 244.

The `JSONRPCReplyObj` signature is:

```c++
UniValue JSONRPCReplyObj(UniValue result, UniValue error, std::optional<UniValue> id, JSONVersion json_version);
```

So without `std::move(e)`, the `error` parameter will be initialized by making a copy of `e` instead of moving from `e`. This is because `e` is an lvalue, while
...

👍 rkrux approved a pull request: "test: Assumeutxo: ensure failure when importing a snapshot twice"
(https://github.com/bitcoin/bitcoin/pull/29973#pullrequestreview-2045916735)
tACK [b259b0e](https://github.com/bitcoin/bitcoin/pull/29973/commits/b259b0e8d360726b062c4b0453d1cf5a68e1933f)

Make successful, all functional tests pass.
Short and to the point PR.

💬 rkrux commented on pull request "test: Assumeutxo: ensure failure when importing a snapshot twice":
(https://github.com/bitcoin/bitcoin/pull/29973#discussion_r1594183450)
I also tested with putting this block before the above for loop, and the tests passes. I wanted to see what would happen if we load another snapshot right after loading one.

💬 instagibbs commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1594188338)
update: in master we're now processing these `PCKG_POLICY` cases, so should be ok? @glozow

💬 luke-jr commented on pull request "RPC: Return `permitbaremultisig` and `maxdatacarriersize` in `getmempoolinfo`":
(https://github.com/bitcoin/bitcoin/pull/29954#issuecomment-2100795137)
Actually, since these don't change on their own... maybe a separate RPC altogether? (later extended to allow some changes?)

💬 luke-jr commented on pull request "net: Replace libnatpmp with built-in PCP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#issuecomment-2100841942)
Would prefer this in two steps (add PCP, then remove NAT-PMP)

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