🤔 hodlinator reviewed a pull request: "refactor: migrate `bool GetCoin` to return `optional<Coin>`"
(https://github.com/bitcoin/bitcoin/pull/30849#pullrequestreview-2311839788)
Drive-by minimum-context, admittedly low-PoW review, prompted by author.

Dislike adding of new TODO's but if full-context reviewers accept, I guess it's okay.

💬 hodlinator commented on pull request "refactor: migrate `bool GetCoin` to return `optional<Coin>`":
(https://github.com/bitcoin/bitcoin/pull/30849#discussion_r1764547631)
++braces, or remove the `else`.

💬 hodlinator commented on pull request "refactor: migrate `bool GetCoin` to return `optional<Coin>`":
(https://github.com/bitcoin/bitcoin/pull/30849#discussion_r1764551012)
\*taps sign\*

💬 hodlinator commented on pull request "refactor: migrate `bool GetCoin` to return `optional<Coin>`":
(https://github.com/bitcoin/bitcoin/pull/30849#discussion_r1764529470)
developer-notes.md:
> If an `if` only has a single-statement `then`-clause, it can appear
on the same line as the `if`, without braces. In every other case,
braces are required, and the `then` and `else` clauses must appear
correctly indented on a new line.

💬 hodlinator commented on pull request "refactor: migrate `bool GetCoin` to return `optional<Coin>`":
(https://github.com/bitcoin/bitcoin/pull/30849#discussion_r1764556394)
\*taps sign\*

💬 hodlinator commented on pull request "refactor: migrate `bool GetCoin` to return `optional<Coin>`":
(https://github.com/bitcoin/bitcoin/pull/30849#discussion_r1764538629)
Avoid inflating the line count?
```suggestion
std::optional<Coin> coin;
if (!mempool || !mempool->isSpent(vOutPoint)) coin = view.GetCoin(vOutPoint);
hits.push_back(coin.has_value());
if (coin) outs.emplace_back(std::move(*coin));
```

💬 Sjors commented on pull request "net, net_processing: additional and consistent disconnect logging":
(https://github.com/bitcoin/bitcoin/pull/28521#discussion_r1764589040)
Fixed.

💬 Sjors commented on pull request "net, net_processing: additional and consistent disconnect logging":
(https://github.com/bitcoin/bitcoin/pull/28521#discussion_r1764589112)
You're right. I dropped this line and checked again that anytime `fDisconnect` is set to `true` emits a log message.

💬 Sjors commented on pull request "net, net_processing: additional and consistent disconnect logging":
(https://github.com/bitcoin/bitcoin/pull/28521#issuecomment-2357781244)
Rebased and dropped duplicate message in `CConnman::DisconnectNodes`.

💬 Sjors commented on pull request "p2p: Increase tx relay rate":
(https://github.com/bitcoin/bitcoin/pull/28592#issuecomment-2357789198)
I've been running this patch for almost a year now. Didn't do very extensive monitoring, but at least it didn't crash the node or noticeably upset the machine.

💬 Sjors commented on pull request "Remove Taproot activation height":
(https://github.com/bitcoin/bitcoin/pull/26201#issuecomment-2357794441)
Removed ~ from the description (wasn't relevant anymore anyway), since @jonatack pointed out in #28358 that these don't get rendered nicely in merges.

💬 Sjors commented on pull request "validation: log which peer sent us a header":
(https://github.com/bitcoin/bitcoin/pull/27826#issuecomment-2357797639)
Post cmake rebase, just in case.

💬 hebasto commented on pull request "depends: Qt 5.15.15":
(https://github.com/bitcoin/bitcoin/pull/30774#issuecomment-2357804575)
> Seems fine to do, but wasn't the idea to use qt6 once cmake was done? IIRC there was a tracking/meta issue, but I can't find it?

I'm working on migration to Qt 6. I'll post a few updates soon.

💬 maflcko commented on pull request "cli: Improve error message on multiwallet cli-side commands":
(https://github.com/bitcoin/bitcoin/pull/26990#issuecomment-2357810004)
review ACK 54227e681a4efa8961f1ad05d43366d88a9b686a

💬 maflcko commented on pull request "cmake: Revamp handling of data files for `{test,bench}_bitcoin` targets":
(https://github.com/bitcoin/bitcoin/pull/30901#discussion_r1764614217)
Ok, fair enough. I just wanted to mention it. Seems fine either way.

💬 Zk2u commented on pull request "Extend signetchallenge to set target block spacing":
(https://github.com/bitcoin/bitcoin/pull/29365#issuecomment-2357864327)
Hi @starius, we've been testing this internally but can't seem to get our blocks below the 2m29s interval. Here's the signet challenge:

```
6a4c090105000000000000004c25512102efc19c310fd60351b84273392d5bc2c58746bd3e8deca043623c0a16065ec2ec51ae
```

Or:

```
6a
4c
09
01 0500000000000000 (signet params: 0x01, pow_target_spacing=5)
4c
25
512102efc19c310fd60351b84273392d5bc2c58746bd3e8deca043623c0a16065ec2ec51ae
```

Calibrated nbits via:
```
../../contrib/signet/miner --cli=".
...

💬 maflcko commented on pull request "doc: Drop description of LogError messages as fatal":
(https://github.com/bitcoin/bitcoin/pull/30361#issuecomment-2357864794)
> So I think either the status quo or #30364 is better than this PR.

I tend to agree on a second thought. This pull just seems to increase the long-term confusion by treating the levels equivalent or similar, just because some parts of the code happen to confuse them (for various reasons).

Also, as pointed out in https://github.com/bitcoin/bitcoin/pull/30364#discussion_r1711345479 (and other comments) a good chunk of the log statements that this doc-only pull is trying to "fix" shouldn't b
...

💬 fanquake commented on issue "ci: ConnectionRefusedError: [WinError 10061] No connection could be made because the target machine actively refused it":
(https://github.com/bitcoin/bitcoin/issues/30390#issuecomment-2357907052)
https://github.com/bitcoin/bitcoin/actions/runs/10907936867/job/30272762595?pr=30918#step:12:226

💬 fanquake commented on pull request "cmake: Switch to crc32c upstream build system":
(https://github.com/bitcoin/bitcoin/pull/30905#issuecomment-2357931815)
Ok. Well the current changes would be unsafe to merge, and I don't think we should be adjusting our build system to work around the subtree, if anything, we should just change the subtree directly.

💬 marcofleon commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1764693774)
Makes sense. Don't feel strongly either, was asking for my own understanding. I meant the `RelayTransaction` comment as a separate topic, referring to transaction types (uint256 vs Txid/Wxid) in general, not specific to this line.