💬 fanquake commented on pull request "docs: improve development guidelines for PR merging":
(https://github.com/bitcoin/bitcoin/pull/32006#issuecomment-2812661425)
@suiyuan1314 what's the status of this? Concept ~0.

💬 fanquake commented on pull request "fuzz: enable running fuzz test cases in Debug mode":
(https://github.com/bitcoin/bitcoin/pull/32113#issuecomment-2812663946)
cc @dergoegge

🤔 l0rinc requested changes to a pull request: "util: explicitly close all AutoFiles that have been written"
(https://github.com/bitcoin/bitcoin/pull/29307#pullrequestreview-2775379592)
Concept ACK, we definitely want to know if this happens!

* It seems we're using the return value as a boolean - can make make it return one?
* The error messages seem all over the place, can we unify them to `LogError`?
* Not sure if scope-ing the AutoFile + closing it explicitly is the best solution - would probably prefer @ryanofsky's `std::function<void()> on_error` or @maflcko's recommendation to let `AutoFile` guarantee proper closing.
* Sometimes we're explicitly showing a closing r
...

💬 l0rinc commented on pull request "util: explicitly close all AutoFiles that have been written":
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r2048754753)
I'd either delete this now or put it before the actual closing. And do we really need to make this unlikely scenario occupy so much space? And in the other case we simply used `LogError` instead:
```C++
// Make sure that the file is closed before we call `FlushUndoFile`.
if (file.fclose()) {
LogError("Failed to close block undo file %s: %s", pos.ToString(), SysErrorString(errno));
return FatalError(m_opts.notifications, state, _("Failed to close block undo file."));
}
```

💬 l0rinc commented on pull request "util: explicitly close all AutoFiles that have been written":
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r2048761555)
nit:
```suggestion
throw std::runtime_error{strprintf("Error closing XOR key file %s: %s",
```

💬 l0rinc commented on pull request "util: explicitly close all AutoFiles that have been written":
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r2048729097)
If we can't fully commit to disk, we won't call `fclose` directly - we'll rely on the destructor's close.
We'd get a commitment failure message (though that could have been successful) - and maybe another message from the destructor.

Could we separate the two errors?
```C++
if (!file.Commit()) {
LogError("%s: Failed to commit filter file %d", __func__, pos.nFile);
return false;
}
if (file.fclose()) {
LogError("%s: Failed to close filter file %d", __func__, pos.nFile);

...

💬 l0rinc commented on pull request "util: explicitly close all AutoFiles that have been written":
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r2048754656)
nit: now that we're closing explicitly we probably don't need the scope, too (wouldn't we double-log in that case anyway?) Or if we keep it, could make sense to add the closing inside - unless it's deliberate, in which case we might want to explain it in the commit message

💬 l0rinc commented on pull request "util: explicitly close all AutoFiles that have been written":
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r2048747378)
Why is this a `LogInfo`, shouldn't it be a `LogError`?
```suggestion
if (fileout.fclose()) {
LogError("%s: Failed to close filter file %d: %s", __func__, pos.nFile, SysErrorString(errno));
return 0;
}
```

💬 l0rinc commented on pull request "util: explicitly close all AutoFiles that have been written":
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r2048769760)
why do we want the destructor to be a noop?

💬 l0rinc commented on pull request "util: explicitly close all AutoFiles that have been written":
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r2048772120)
nit: this likely isn't strictly necessary, as long as it contains all the data (checked below), it should work correctly - but it's fine either way

💬 l0rinc commented on pull request "util: explicitly close all AutoFiles that have been written":
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r2048791750)
```suggestion
LogError("Failed to close file: %s", SysErrorString(errno));
```

💬 l0rinc commented on pull request "util: explicitly close all AutoFiles that have been written":
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r2048785183)
Can you separate the `AutoFile` changes to a new commit, explaining these changes separately from the changes that are calling close explicitly?

💬 hebasto commented on pull request "depends: bump boost to 1.87.0 and use new CMake buildsystem":
(https://github.com/bitcoin/bitcoin/pull/30434#issuecomment-2812702334)
> > @hebasto Want to try putting that behind a variable and upstreaming it? It'd be nice if we didn't have to carry that patch.
>
> Sure thing!
>
> Done in [boostorg/test#445](https://github.com/boostorg/test/pull/445).

[Here](https://github.com/hebasto/bitcoin/commits/250417-boost-cmake/) is an updated branch based on the upstream changes.

💬 ismaelsadeeq commented on issue "Test Package Accept":
(https://github.com/bitcoin/bitcoin/issues/32160#issuecomment-2812704410)
Given that we want `testmempoolaccept` to relax the requirement that the transaction should be topologically connected due to the reasons discussed above, should we consider extending `submitpackage` to do the same since as you mentioned in practice, some transactions already in the mempool can cause the submitted package to become topological, the submitted package may not appear connected the outset because it was deduplicated (but it is with some in-mempool transaction).

> This allows us to
...

💬 l0rinc commented on pull request "[IBD] specialize block serialization":
(https://github.com/bitcoin/bitcoin/pull/31868#issuecomment-2812715464)
> but I am not really reproducing any of the performance changes on my machine yet

I just removed the single-byte specializations, they were quite ugly, will have to add it back somehow later - will draft it until I do that, thanks for checking!

📝 l0rinc converted_to_draft a pull request: "[IBD] specialize block serialization"
(https://github.com/bitcoin/bitcoin/pull/31868)
This change is part of [[IBD] - Tracking PR for speeding up Initial Block Download](https://github.com/bitcoin/bitcoin/pull/32043)

### Summary

This PR contain a few different optimization I found by IBD profiling, and via the newly added block seralization benchmarks. It also takes advantage of the recently merged [`std::span` changes](https://github.com/bitcoin/bitcoin/pull/31519) enabling propagating static extents.

The commits merge similar (de)serialization methods, and separates th
...

🚀 fanquake merged a pull request: "fuzz: Avoid integer sanitizer warnings in policy_estimator target"
(https://github.com/bitcoin/bitcoin/pull/32154)

💬 Sjors commented on pull request "Add bitcoin-{node,gui} to release binaries for IPC":
(https://github.com/bitcoin/bitcoin/pull/31802#discussion_r2048861716)
Expanded the comment and commit description.

💬 Sjors commented on pull request "Add bitcoin-{node,gui} to release binaries for IPC":
(https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-2812748207)
Rebased after #32271.

💬 fanquake commented on pull request "ci: switch to LLVM 20 in tidy job":
(https://github.com/bitcoin/bitcoin/pull/32226#issuecomment-2812748458)
@l0rinc I've taken some of these, but I'm not planning on making further refactors here, just updating the infra. Happy to review a followup.