💬 theuni commented on pull request "util: add BitSet":
(https://github.com/bitcoin/bitcoin/pull/30160#discussion_r1612232560)
`std::bitset` throws for these if pos is out of range.

Maybe add `Assume()`s? Or were you trying to keep the header self-contained?

💬 apulsifer commented on issue "LevelDB read failure: Corruption: block checksum mismatch":
(https://github.com/bitcoin/bitcoin/issues/30159#issuecomment-2127922118)
> Could you explain the paging in a bit more detail?

The machines page pretty hard to SSD for about 20 seconds after each new block arrives. That info comes from "sar -d 10", which I logged for a while when I was setting up the first machine.

> Is the node constantly being bombarded with lots of simultaneous RPC calls (which ones?) or is some other interface used?

None of these machines has at this point serviced a single RPC call (I'm still trying to get things set up). No other inter
...

💬 theuni commented on pull request "util: add BitSet":
(https://github.com/bitcoin/bitcoin/pull/30160#discussion_r1612240218)
I know you like dense code, but...

I suspect function names would make reviewing uses of this easier than operators. Especially for the non-obvious ones like `&&` that `std::bitset` doesn't support.

I know that personally I'd have an easier time reviewing `a.intersects(b)`.

💬 epiccurious commented on pull request "doc: Update mentions of generating bitcoin.conf":
(https://github.com/bitcoin/bitcoin/pull/30154#discussion_r1612244540)
Agreed. Resolved with aef43a3a996618d581e0a16f7fc5e842708e83c9.

💬 davidgumberg commented on pull request "doc: update mention of generating bitcoin.conf":
(https://github.com/bitcoin/bitcoin/pull/30154#issuecomment-2127946115)
You should squash this into a single commit and update the PR description

💬 maflcko commented on issue "LevelDB read failure: Corruption: block checksum mismatch":
(https://github.com/bitcoin/bitcoin/issues/30159#issuecomment-2127953334)
I think calling the RPC `gettxoutsetinfo muhash` on all nodes (when they are synced to the same block) and it matches for all, then the chainstate leveldb at that point in time is probably fine. I presume all failures happened in the `/bdata/bitcoin-data/chainstate/` leveldb?

💬 maflcko commented on pull request "ci: add markdown link check job":
(https://github.com/bitcoin/bitcoin/pull/30034#issuecomment-2127974812)
> * That being said, this holds for all of the existing linter dependencies, and this dependency is optional and I am not an expert in security.

Right, the pull here does not change that. I'd say, it is best to only run the linters locally in a sandbox, or at least skip the optional dependencies.

💬 jonatack commented on pull request "net: add ASMap info in `getrawaddrman` RPC":
(https://github.com/bitcoin/bitcoin/pull/30062#discussion_r1612272793)
Why is `connman` passed by reference, and not reference to const? It isn't modified.

💬 jonatack commented on pull request "net: add ASMap info in `getrawaddrman` RPC":
(https://github.com/bitcoin/bitcoin/pull/30062#discussion_r1612273177)
Same comment here as https://github.com/bitcoin/bitcoin/pull/30062/files#r1612272793

💬 jonatack commented on pull request "net: add ASMap info in `getrawaddrman` RPC":
(https://github.com/bitcoin/bitcoin/pull/30062#discussion_r1612276581)
Using `auto` here requires looking up `GetMappedAS` to know the type. Using the actual type in this case would have been clearer to the reader. The conditional could also have been simpler, though that's arguably a style nit.

```diff
- const auto mapped_as{connman.GetMappedAS(info)};
- if (mapped_as != 0) {
+ const uint32_t mapped_as{connman.GetMappedAS(info)};
+ if (mapped_as) {
```

💬 jonatack commented on pull request "net: add ASMap info in `getrawaddrman` RPC":
(https://github.com/bitcoin/bitcoin/pull/30062#discussion_r1612276973)
Same comment here as https://github.com/bitcoin/bitcoin/pull/30062/files#r1612276581.

💬 maflcko commented on pull request "build: LLD based macOS toolchain":
(https://github.com/bitcoin/bitcoin/pull/21778#issuecomment-2127983803)
I presume macos-cross compilation on risv64 metal still fails? If it is expected to pass now, I can re-try this.

💬 jonatack commented on pull request "net: add ASMap info in `getrawaddrman` RPC":
(https://github.com/bitcoin/bitcoin/pull/30062#discussion_r1612279756)
Here and in the other new help below, the reason for the optional nature of this field could perhaps be mentioned (as simply as this ", if present", or perhaps in more detail).

```suggestion
{RPCResult::Type::NUM, "mapped_as", /*optional=*/true, "The ASN mapped to the IP of this peer per our current ASMap, if present"},
```

💬 LarryRuane commented on pull request "improve MallocUsage() accuracy":
(https://github.com/bitcoin/bitcoin/pull/28531#issuecomment-2128007110)
Your patch is much better, @theStack, thanks! It's simpler and doesn't need that weird `sleep`. Force-pushed your patch. If it passes CI, I think this is the way to go. I had tried something similar, but I didn't think to make the sending of that extra (smaller) transaction conditional based on available mempool space.

💬 brunoerg commented on pull request "net: add ASMap info in `getrawaddrman` RPC":
(https://github.com/bitcoin/bitcoin/pull/30062#discussion_r1612300529)
Makes sense, I will address it in a follow-up PR.

💬 brunoerg commented on pull request "net: add ASMap info in `getrawaddrman` RPC":
(https://github.com/bitcoin/bitcoin/pull/30062#issuecomment-2128020594)
Thanks for reviewing, @jonatack. I'm working on a follow-up.

💬 jonatack commented on pull request "i2p: fix and improve logs":
(https://github.com/bitcoin/bitcoin/pull/29833#discussion_r1612315058)
> Note that for transient I2P sessions (when `-i2pacceptincoming=0`) this would be printed for every I2P peer connect and disconnect even if I2P logging is switched off.

Good point. At the same time, I2P connections are intended to be long-running ones, as the tunnels are one-way and more time/resource intensive to launch than other networks. I'm unsure whether it's best for this to be verbose, or to set it as a less-frequent log level if `i2pacceptincoming` is off.

💬 jonatack commented on pull request "ci: add markdown link check job":
(https://github.com/bitcoin/bitcoin/pull/30034#discussion_r1612322539)
Concept ACK on additional checks.

Could this have been an individual python lint script like the other ones in `/test/lint/`? I appreciated being able to run individual python scripts locally to check my work or when reviewing. It seems we have been losing that recently.

👍 theStack approved a pull request: "refactor prep for package rbf"
(https://github.com/bitcoin/bitcoin/pull/30072#pullrequestreview-2075196742)
re-ACK 2fd34ba504957331f5a08614b6e1f8317260f04d

💬 jonatack commented on pull request "i2p: fix and improve logs":
(https://github.com/bitcoin/bitcoin/pull/29833#issuecomment-2128122156)
I would squash the first and last commits, as the latter is just retouching 2 lines already changed in the first commit, to `i2p: log with LogPrintLevel per severity level`