💬 jonatack commented on pull request "refactor: Remove redundant and confusing calls to IsArgSet":
(https://github.com/bitcoin/bitcoin/pull/31896#discussion_r2010672448)
fa41685f438bba00761 For the 3 conditionals dropped in this commit, any reason to keep the scope braces?

💬 jonatack commented on pull request "refactor: Remove redundant and confusing calls to IsArgSet":
(https://github.com/bitcoin/bitcoin/pull/31896#discussion_r2010685447)
For all `GetArg` calls in this commit. Perhaps I am confused but should be const ref? Would also be more clear to continue to specify the type here.

```suggestion
if (const std::string& port{args.GetArg(port_option)}) {
```

💬 jonatack commented on pull request "refactor: Remove redundant and confusing calls to IsArgSet":
(https://github.com/bitcoin/bitcoin/pull/31896#discussion_r2010690560)
FWIW these two lines are from daec955fd68b (https://github.com/bitcoin/bitcoin/pull/9380)

💬 marcofleon commented on pull request "refactor: Enforces Txid and Wtxid types in RelayTransaction":
(https://github.com/bitcoin/bitcoin/pull/32104#issuecomment-2749104911)
Lgtm as a standalone change. It's the same change I implement in fa8400494e3c411a55375e71abaaa2637a22e216 as part of the complete refactor. I actually missed all the `tx.GetHash()` and `tx.GetWitnessHash()` in `ProcessMessage` so good catch there.

My [final refactor](https://github.com/bitcoin/bitcoin/compare/master...marcofleon:bitcoin:workingtxidtypesafety) does end up altering `RelayTransaction` in a more involved way, but that includes changes (switching `GenTxid` to a variant) that haven
...

💬 johnnyasantoss commented on issue "bitcoin core crashes when too many rpc calls are made":
(https://github.com/bitcoin/bitcoin/issues/11368#issuecomment-2749116445)
We've experienced this bug, using a clean ubuntu installation. We had a transaction scanner running for some transactions on a new block, but by a mistake we ran the scanner on all txs on the block. Ideally core should never crash but start rejecting requests and stop enqueuing work on the rpc queue.

```
2025-03-20T18:46:57Z [libevent:warning] Error from accept() call: Too many open files
2025-03-20T18:46:57Z [libevent:warning] Error from accept() call: Too many open files
2025-03-20T18:46:57Z
...

💬 yancyribbens commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r2010820383)
This file has some strange formatting that does not show up in Github. My editor (Vim) just shows a character that seems like a translation error.

> 5 sat

I opened the file with `xxd` and the translation is `35e2 80af 7361 742f`. `35` is the hex for `5` in ascii, and `73` is the hex for `s`. It looks like `80` is extended hex for the euro sign and `e2` and `af` also have extended hex meanings. There are more of these weird encodings thought the file.

💬 mzumsande commented on pull request "validation: Add eligible ancestors of reconsidered block to setBlockIndexCandidates":
(https://github.com/bitcoin/bitcoin/pull/30479#discussion_r2010827029)
updated, thanks!

💬 murchandamus commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r2010835264)
That seems like a good idea, but that’s out of scope for this PR. I have had a refactor of BnB on my todo list for a long time, and it would be part of that, probably.

💬 mzumsande commented on pull request "validation: Add eligible ancestors of reconsidered block to setBlockIndexCandidates":
(https://github.com/bitcoin/bitcoin/pull/30479#issuecomment-2749245050)
[4a76a83](https://github.com/bitcoin/bitcoin/commit/4a76a8300f295e8eab0ea8ab1ea1580450f131c8) to [adcbb2a](https://github.com/bitcoin/bitcoin/commit/adcbb2a32e60aa49faff0c41033adcc149336220):

- Rebased onto master because the CI failed, as far as I can see unrelated to the PR.
- Added test by @stratospher, thanks! (with minor changes, no need to re-format `block.sha256`, can just use `block.hash`).
- Added a commit that replaces `IsValid()` by a comparison with `BLOCK_FAILED_MASK`. `IsValid
...

🤔 sipa reviewed a pull request: "contrib: document asmap-tool commands more thoroughly"
(https://github.com/bitcoin/bitcoin/pull/32110#pullrequestreview-2711540347)
ACK fa52e606de7a12921619b56a631cd194e2366cc1

💬 1440000bytes commented on pull request "Rust tool to import bip39 mnemonic":
(https://github.com/bitcoin/bitcoin/pull/32115#issuecomment-2749251585)
Concept ACK

> I don't think we should tell users to just google for utilities like this. Nor should we link to some project that we're not reviewing. And if we don't tell users where it is, then only people who've heard of rust-bitcoin will use it. I wouldn't consider that a fix of #19151.

Agree. This could be a useful tool for bitcoin core users.

🤔 ryanofsky reviewed a pull request: "[IBD] batch block reads/writes during `AutoFile` serialization"
(https://github.com/bitcoin/bitcoin/pull/31551#pullrequestreview-2711550079)
Code review 2c073b90ae977af1ac01164a894edd978529fdc6. I think the new BufferedFile approach implemented is reasonable and a little nicer than the [previous](https://github.com/bitcoin/bitcoin/pull/31551#issuecomment-2722752125) approach which made lower-level calls in block storage code to allocate and pass buffers. But the need to hardcode and compute sizes in 8782d6ed09bbb947426c1ed54ad4d5188326764d and 2c073b90ae977af1ac01164a894edd978529fdc6 still seems to make the storage code more awkward
...

🚀 ryanofsky merged a pull request: "test: Add expected result assertions"
(https://github.com/bitcoin/bitcoin/pull/31656)

🤔 ryanofsky reviewed a pull request: "doc: Update documentation to include Clang/llvm based coverage report generation"
(https://github.com/bitcoin/bitcoin/pull/31933#pullrequestreview-2711577943)
@Prabhat1308 this seems ready to merge but also has a few recent comments that haven't been responded to. You can let me know if you'd like this to be merged or if you want to make more changes.

👍 hodlinator approved a pull request: "build: Remove bitness suffix from Windows installer"
(https://github.com/bitcoin/bitcoin/pull/32132#pullrequestreview-2711591169)
ACK fb2b05b1259d3e69e6e675adfa30b429424c7625

We still have things like
x86_64-w***64***-mingw***32***/bitcoin-94967c353ed8-win***64***-setup-unsigned.exe
in build output, but I understand wanting to defer those until later.

🤔 marcofleon reviewed a pull request: "fuzz: coinselection: cover `SetBumpFeeDiscount`"
(https://github.com/bitcoin/bitcoin/pull/31806#pullrequestreview-2711621352)
ACK 0ff66b1c4ab0e5cba00a178b24f2c5de75df360f

Checked for the additional coverage to be sure, lgtm

💬 jsarenik commented on issue "Failed transactions on importing mempool":
(https://github.com/bitcoin/bitcoin/issues/32125#issuecomment-2749337486)
Another example of what I was thinking of in the beginning and what is already possible using the script that prioritizes transactions before importing them:

```
newlines added
2025-03-24T20:31:45Z
Imported mempool transactions from file:
1267 succeeded, 0 failed, 0 expired, 29536 already there,
0 waiting for initial broadcast
```

One thing I can think of right now would be an RPC which clears all prioritization. No idea how to do it in Bitcoin Core yet, but the idea is here.

💬 murchandamus commented on issue "BnB untested/unused condition in UTXO exclusion optimization":
(https://github.com/bitcoin/bitcoin/issues/32047#issuecomment-2749339392)
> That would be great to update BnB to be more like coin-grinder. Adding an iteration count would be a good first step towards that.

I don’t think it makes sense to add just the iteration count, if it should be refactored completely anyway.—Better to do it all in one.

> > I could have sworn we had such a test already, but if we don’t that would be a great addition to the [bnb_feerate_sensitivity_test](https://github.com/bitcoin/bitcoin/pull/29532/files#diff-36088c93368e137d955348aba223985bd4f1
...

💬 davidgumberg commented on pull request "build: Drop option to disable hardening.":
(https://github.com/bitcoin/bitcoin/pull/32071#discussion_r2010894527)
Thanks, fixed

🚀 ryanofsky merged a pull request: "net: Block v2->v1 transport downgrade if !fNetworkActive"
(https://github.com/bitcoin/bitcoin/pull/32073)