💬 Sjors commented on pull request "doc, chainparams: 29775 release notes and follow-ups":
(https://github.com/bitcoin/bitcoin/pull/30604#discussion_r1710919235)
And see #30614.

✅ maflcko closed an issue: "getmempoolentry returns "incorrect" bip125-replaceable status"
(https://github.com/bitcoin/bitcoin/issues/22209)

💬 maflcko commented on issue "getmempoolentry returns "incorrect" bip125-replaceable status":
(https://github.com/bitcoin/bitcoin/issues/22209#issuecomment-2277310259)
Closing for now. This was fixed in commit 902dd14382256c9d33bce667795a64079f3bee6b.

Well, in theory, it is still possible to set `mempoolfullrbf=0` and then reproduce this "bug", but anyone fiddling with policy must know exactly what they are doing.

In any case, I don't think there is anything left that can be done here. (Other than completely removing the setting, but this is already done in https://github.com/bitcoin/bitcoin/pull/30592, so let's continue the discussion there)

💬 Sjors commented on issue "UpdateTime() needs to account for timewarp fix on testnet4 ":
(https://github.com/bitcoin/bitcoin/issues/30614#issuecomment-2277313868)
Although I think we should fix this, it seems potentially problematic to expect other node / miner implementations to also fix this. That's something to consider in the discussion of whether to change the 7200 value.

🤔 maflcko reviewed a pull request: "Remove mempoolfullrbf"
(https://github.com/bitcoin/bitcoin/pull/30592#pullrequestreview-2229526483)
left a RPC doc nit, feel free to ignore

💬 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).