💬 0xB10C commented on pull request "net: add ASMap info in `getrawaddrman` RPC":
(https://github.com/bitcoin/bitcoin/pull/30062#issuecomment-2100699418)
Cool! Concept ACK.

I've added prelimarly support for this in the static HTML page on https://addrman.observer. You can now load the new `getrawaddrman` dumps and it will show you the AS and source AS. You can also mouse-over highlight by AS and source ASN.


💬 ryanofsky commented on pull request "kernel: Remove key module from kernel library":
(https://github.com/bitcoin/bitcoin/pull/29252#discussion_r1594092791)
re: https://github.com/bitcoin/bitcoin/pull/29252#discussion_r1593014537

> A RAII wrapper for the ECC state sounds like a nice improvement.

You probably saw this, but just for anyone else reading, this is implemented in the [branch](https://github.com/ryanofsky/bitcoin/commits/pr/ecc) posted https://github.com/bitcoin/bitcoin/pull/29252#pullrequestreview-2042988944

💬 ryanofsky commented on pull request "kernel: Remove key module from kernel library":
(https://github.com/bitcoin/bitcoin/pull/29252#discussion_r1594093371)
re: https://github.com/bitcoin/bitcoin/pull/29252#discussion_r1593012869

> Some of the option fields do currently hold references and it might be a good idea to move ownership of those referred values to the kernel Context eventually.

Fair enough, new members could be added and kernel::Context could become something more like what was originally described, and the new comment I suggested would need to get updated in that case. To be sure, I don't think any changes need to be made as part o
...

💬 0xB10C commented on pull request "net: add ASMap info in `getrawaddrman` RPC":
(https://github.com/bitcoin/bitcoin/pull/30062#discussion_r1594126015)
nit: I always found this confusing as I read `mapped as <something>` and not `mapped *A*utonomous*S*ystem` - don't have a concrete idea for a name and I guess it makes sense to use the same as in `getpeerinfo`.

💬 luke-jr commented on pull request "RPC: Return `permitbaremultisig` and `maxdatacarriersize` in `getmempoolinfo`":
(https://github.com/bitcoin/bitcoin/pull/29954#issuecomment-2100723507)
I wonder if a sub-object would be a good idea before we start adding more settings here. There's at least also `m_expiry` that would make sense to add too.

💬 sr-gi commented on pull request "fuzz: txorphan tests fixups":
(https://github.com/bitcoin/bitcoin/pull/29974#issuecomment-2100723722)
@maflcko would you mind taking a look at this (given you reviewed the original PR for this fuzz test)?

💬 luke-jr commented on pull request "RPC: Return `permitbaremultisig` and `maxdatacarriersize` in `getmempoolinfo`":
(https://github.com/bitcoin/bitcoin/pull/29954#discussion_r1594145863)
Well, you added "max" in front. I guess it makes sense, but might be surprising.

It's not clear what compatibility would mean before Core has fixed it to actually work.

💬 mzumsande commented on issue "Consideration of a Code of Conduct and/or written rules for moderation":
(https://github.com/bitcoin/bitcoin/issues/29507#issuecomment-2100732287)
> Also: I would like to propose someone opens an issue for follow-up discussion in the meta repo, links to that issue here, and then closes this issue. I think moving meta talk to meta makes sense.

There already exists one: https://github.com/bitcoin-core/meta/issues/1

💬 sipa commented on issue "libxcb-xinerama0 Library required by bitcoin-qt":
(https://github.com/bitcoin/bitcoin/issues/30061#issuecomment-2100737176)
@nimrare The short answer here is that there is no way around trusting your operating system's libraries. Even if all userspace things would be statically linked, you're still relying on your kernel for example. And it turns out that for interacting with graphics subsystems of your operating system, dynamic libraries are practically the only solution, as statically-linked ones would pretty much only work on the exact system they were compiled for.

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