🤔 maflcko reviewed a pull request: "Fixes for GCC 15 compatibility"
(https://github.com/bitcoin/bitcoin/pull/30612#pullrequestreview-2229402700)
> * Two are related to missing includes. You can't use `uint16_t` et al. without including `<cstdint>`.

Probably a good time to enable iwyu. :sweat_smile:

> This manifests as the following compile-time mess:

Are you sure this is not a GCC bug?

💬 maflcko commented on pull request "Fixes for GCC 15 compatibility":
(https://github.com/bitcoin/bitcoin/pull/30612#discussion_r1710836374)
```suggestion
template<std::integral I> // Disallow silent float -> int conversion
```

Please keep the comment

💬 maflcko commented on pull request "Fixes for GCC 15 compatibility":
(https://github.com/bitcoin/bitcoin/pull/30612#discussion_r1710833459)
iwyu:

```
node/interface_ui.h should add these lines:
#include <vector> // for vector

node/interface_ui.h should remove these lines:
- #include <memory> // lines 11-11

💬 whitslack commented on pull request "Fixes for GCC 15 compatibility":
(https://github.com/bitcoin/bitcoin/pull/30612#discussion_r1710848701)
With all due respect, that's a semantically void comment. `std::integral I` already means "disallow floats."

💬 whitslack commented on pull request "Fixes for GCC 15 compatibility":
(https://github.com/bitcoin/bitcoin/pull/30612#issuecomment-2277260451)
> Are you sure this is not a GCC bug?

The compile-time error is of a class of errors that is not specific to GCC. Maybe the stdlibc++ in GCC 15 changed the implementation of `std::optional` so that the previously valid code in `feerate.h` is no longer valid. In general, if, in the course of checking a type constraint, the compiler instantiates a template that invokes the same type constraint, then there is an infinite recursion, and the compiler will reject the code with an error saying that
...

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

<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 2925bd537cbd8c70594e23f6c4
...

💬 Sjors commented on pull request "doc, chainparams: 29775 release notes and follow-ups":
(https://github.com/bitcoin/bitcoin/pull/30604#discussion_r1710890448)
35ad61a6bde14a74cc07dc8ecf3bb1cd49590e46: let's define a separate constant and make it clear these are different but related:

```cpp
/**
* Maximum number of seconds that the timestamp of the first
* block of a difficulty adjustment period is allowed to
* be earlier than the last block of the previous period.
*/
static constexpr int64_t MAX_TIMEWARP{MAX_FUTURE_BLOCK_TIME};

/**
* If the timestamp of the last block in a difficulty period is set
* MAX_FUTURE_BLOCK_TIME seconds in
...

💬 maflcko commented on pull request "Fixes for GCC 15 compatibility":
(https://github.com/bitcoin/bitcoin/pull/30612#issuecomment-2277287178)
> > Are you sure this is not a GCC bug?
>
> The compile-time error is of a class of errors that is not specific to GCC. Maybe the stdlibc++ in GCC 15 changed the implementation of `std::optional` so that the previously valid code in `feerate.h` is no longer valid.

Correct. Though, GCC 15 is so far out that there is a chance the change will be reverted again internally, because I assume it breaks compilation of more code than just this single instance. I am wondering if their change was int
...

⚠️ Sjors opened an issue: "UpdateTime() needs to account for timewarp fix on testnet4 "
(https://github.com/bitcoin/bitcoin/issues/30614)
In (`miner.cpp`) we set the block (template) timestamp as follows:

```cpp
pblock->nTime = TicksSinceEpoch<std::chrono::seconds>(NodeClock::now());
...
UpdateTime(pblock, ...)


int64_t UpdateTime(CBlockHeader* pblock, ...)
{
int64_t nOldTime = pblock->nTime;
int64_t nNewTime{std::max<int64_t>(pindexPrev->GetMedianTimePast() + 1, TicksSinceEpoch<std::chrono::seconds>(NodeClock::now()))};

if (nOldTime < nNewTime) {
pblock->nTime = nNewTime;
}
```

In oth
...

💬 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