💬 maflcko commented on pull request "Remove mempoolfullrbf":
(https://github.com/bitcoin/bitcoin/pull/30592#discussion_r1710942068)
RPC doc-nit: Could mention this field is "deprecated", because it returns a constant and only kept for historic reasons to avoid changing the RPC interface now?

There is almost no cost to keeping it, but I think at some point in the future, it can be removed.

Same for the `bip125-replaceable` fields, which also only have historic value to keep around a bit, but can be removed at some point into the future.

🤔 Sjors reviewed a pull request: "doc, chainparams: 29775 release notes and follow-ups"
(https://github.com/bitcoin/bitcoin/pull/30604#pullrequestreview-2229537666)
ACK 35ad61a6bde14a74cc07dc8ecf3bb1cd49590e46 modulo the above comment

💬 Sjors commented on pull request "doc, chainparams: 29775 release notes and follow-ups":
(https://github.com/bitcoin/bitcoin/pull/30604#discussion_r1710950771)
1163b08378a50a9be00ced434d55f1b04bc9dea6: I get the same value

💬 stickies-v commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1710962569)
Yeah, even though this assumption seems safe at the moment it's brittle and blindly dereferencing it is not good practice. Will use `.value()` either in next force-push or in the follow-up PR removing `uint256S` completely. Thanks.

💬 Sjors commented on pull request "assumeutxo: Drop block height from metadata":
(https://github.com/bitcoin/bitcoin/pull/30598#discussion_r1710979911)
I would be fine with keeping the version at 1 since this was never released, the old torrent will probably disappear, and the change here doesn't have backwards compatible support.

👍 stickies-v approved a pull request: "Fixes for GCC 15 compatibility"
(https://github.com/bitcoin/bitcoin/pull/30612#pullrequestreview-2229584264)
> In any case, the new code seems clearer and more correct either way.

I agree. ACK 4fbf83fa864f0f18e5946c3ad665a24df20daf01

💬 Sjors commented on pull request "assumeutxo: Drop block height from metadata":
(https://github.com/bitcoin/bitcoin/pull/30598#discussion_r1710989491)
Also why drop `m_version`? That seems incompatible with the idea of being able to load older versions.

⚠️ dergoegge opened an issue: "fuzz: `script`: Assertion `!extract_destination_ret' failed."
(https://github.com/bitcoin/bitcoin/issues/30615)
```
$ echo "TnNcAALv////Ua2t" | base64 --decode > script.crash
$ FUZZ=script src/test/fuzz/fuzz script.crash
fuzz: test/fuzz/script.cpp:87: void script_fuzz_target(FuzzBufferType): Assertion `!extract_destination_ret' failed.
```

💬 dergoegge commented on issue "fuzz: `script`: Assertion `!extract_destination_ret' failed.":
(https://github.com/bitcoin/bitcoin/issues/30615#issuecomment-2277388310)
This looks P2A related (#30352).

cc @instagibbs @glozow

💬 maflcko commented on pull request "assumeutxo: Drop block height from metadata":
(https://github.com/bitcoin/bitcoin/pull/30598#discussion_r1710995384)
> I would be fine with keeping the version at 1

What is the benefit? An unsigned 16-bit integer should have more than enough versions for any need, so incrementing is close to 0-cost. Even if there was a shortage at some point, re-using `1` at that point in time should be trivially possible for the reasons given by you.

In any case, I am happy to ack either version of `VERSION`.

💬 maflcko commented on pull request "assumeutxo: Drop block height from metadata":
(https://github.com/bitcoin/bitcoin/pull/30598#discussion_r1711003005)
> Also why drop m_version?

The field isn't dropped. It is simply renamed from `m_version` to `VERSION`, keeping everything else the same.

If there is a need to load older versions in the future, the code can be written then. Also, writing such code seems unrelated to this pull request.

💬 maflcko commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#issuecomment-2277446519)
ACK 855784d3a0026414159acc42fceeb271f8a28133 🔋

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: ACK 855784d3a0026414159acc42fc
...

👍 theStack approved a pull request: "assumeutxo: Drop block height from metadata"
(https://github.com/bitcoin/bitcoin/pull/30598#pullrequestreview-2229675032)
Code-review ACK 00618e8745192d209c23e3ae873c077e58168957

💬 theStack commented on pull request "assumeutxo: Drop block height from metadata":
(https://github.com/bitcoin/bitcoin/pull/30598#discussion_r1711058464)
TIL that `inline` can also be applied to variables since C++17 👀

💬 fanquake commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1711061417)
> The problem was that [brew gcc](https://github.com/orgs/Homebrew/discussions/4794#discussioncomment-7044468) and Xcode 15 have an duplicate lib:

Not sure, given that we aren't using Brew GCC, or Xcode, and the machine I tested on doesn't have either of them installed. The warning output might match, but I don't think this is the correct explanation (and it doesn't explain why it doesn't happen for Autotools build).

💬 Sjors commented on pull request "net, net_processing: additional and consistent disconnect logging":
(https://github.com/bitcoin/bitcoin/pull/28521#discussion_r1711062476)
I think we should avoid accessing the global variable `fLogIPs`. A future refactor could add `m_log_ips` to `PeerManagerImpl` and/or `CConnman`, initialize it from `gArgs` and then drop `fLogIPs`.

🚀 fanquake merged a pull request: "C++20 std::views::reverse"
(https://github.com/bitcoin/bitcoin/pull/28687)

💬 TheCharlatan commented on pull request "C++20 std::views::reverse":
(https://github.com/bitcoin/bitcoin/pull/28687#issuecomment-2277481665)
Post-merge ACK 2925bd537cbd8c70594e23f6c4298b7101f7f73d

💬 maflcko commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1711088525)
(In the follow-up you can also adjust the error message to explain the new max-length check. Otherwise, this could be confusing:

`Error: Invalid non-hex (0x1234ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff) minimum chain work value specified`)

💬 fjahr commented on pull request "assumeutxo: Drop block height from metadata":
(https://github.com/bitcoin/bitcoin/pull/30598#discussion_r1711119588)
Yeah, this fixed https://github.com/bitcoin/bitcoin/runs/28432306724