💬 instagibbs commented on pull request "Replace cluster linearization algorithm with SFL":
(https://github.com/bitcoin/bitcoin/pull/32545#issuecomment-3612546191)
glancing through the PR now

how bad would it be to merge all the way through "clusterlin: replace cluster linearization with SFL implementation (feature)", maybe some of the commits that drops unused things (ala "clusterlin: remove unused MergeLinearizations (cleanup)") and defer the other optimisations for next PR?

📝 maflcko opened a pull request: "Add util::Expected (std::expected)"
(https://github.com/bitcoin/bitcoin/pull/34006)

💬 maflcko commented on pull request "util: generalize `util::Result` to support custom errors":
(https://github.com/bitcoin/bitcoin/pull/34005#issuecomment-3612561978)
I wonder if this is the right approach. `util::Result` is mostly meant for high level code (dealing with translations), such as init, the wallet, or the gui. For low-level stuff, it could make sense to use `std::expected` directly?

I am not sure how fast we'll be getting C++23. C++20 was enabled two years ago in https://github.com/bitcoin/bitcoin/pull/28349, but there are still questions around which C++20 features are supported. (e.g. jthread is only in libc++ 20, std::format is only in g++1
...

💬 l0rinc commented on pull request "util: generalize `util::Result` to support custom errors":
(https://github.com/bitcoin/bitcoin/pull/34005#issuecomment-3612570208)
> It might be good if this added an alias like

Good idea, added and used it in the new test cases.

> Have you looked at https://github.com/bitcoin/bitcoin/pull/25665?

I agree with @ryanofsky that this is likely compatible with the PR and as far as I understand it addresses a small chunk of what it's also trying to achieve.

> So I think just porting a minimal std::expected seems easier?
So I did that in https://github.com/bitcoin/bitcoin/pull/34006

I'm fine with both, let me know
...

💬 maflcko commented on pull request "util: generalize `util::Result` to support custom errors":
(https://github.com/bitcoin/bitcoin/pull/34005#issuecomment-3612589376)
> Looking at [bb61eca](https://github.com/bitcoin/bitcoin/commit/bb61eca55a563d7486df3c356d163c4af10a36c8), I think this is compatible with #25665. This PR is a minimal tweak to `util::Result` that can let it work as a substitute for `std::expected` before `std::expected` is available to use. #25665 could be easily rebased on top of this. In the long run, though, I think code that doesn't need `util::Result` error reporting functionality should just use `std::expected` directly.

Not sure they
...

💬 ryanofsky commented on pull request "util: generalize `util::Result` to support custom errors":
(https://github.com/bitcoin/bitcoin/pull/34005#issuecomment-3612602449)
> So I think just porting a minimal `std::expected` seems easier?
>
> So I did that in #34006

Nice, that's more complicated than this PR, but not too bad. I don't have any particular preference between these two PR's. Either seem fine.

💬 ajtowns commented on issue "SENDTEMPLATE Tracking Issue":
(https://github.com/bitcoin/bitcoin/issues/33691#issuecomment-3612640676)
It looks like reusing compact block messages doesn't make sense -- including two blocks' worth of data seems pretty useful, but that could result in an ~8MB `blocktxns` data if your mempool was empty and the top of the mempool was full of inscriptions, which would in turn [exceed the 4MB maximum message size we have](https://github.com/bitcoin/bitcoin/pull/33191#pullrequestreview-3383627846). We could expand the maximum message size, but large messages can be a DoS vector, so splitting the templ
...

💬 maflcko commented on pull request "util: generalize `util::Result` to support custom errors":
(https://github.com/bitcoin/bitcoin/pull/34005#issuecomment-3612682436)
> > So I think just porting a minimal `std::expected` seems easier?
> > So I did that in #34006
>
> Nice, that's more complicated than this PR, but not too bad. I don't have any particular preference between these two PR's. Either seem fine.

Yeah, it is a bit more code, but, the inner guts of `util::Expected` are mostly copy-pasted from the current `util::Result` in master, so it shouldn't be conceptually complicated.

💬 pablomartin4btc commented on pull request "argsman, cli: GNU-style command-line option parsing (allows options after non-option arguments)":
(https://github.com/bitcoin/bitcoin/pull/33540#issuecomment-3612742963)
> > Restructuring `ProcessOptionKey` as below seems to solve the problem...
>
> Checking it... Thanks for the suggestion!

Ok, all tests pass...

First checked only the affected ones by my changes:

```
./build/test/functional/feature_config_args.py
./build/test/functional/tool_wallet.py
./build/test/functional/wallet_multiwallet.py
./build/test/functional/wallet_transactiontime_rescan.py

./build/bin/test_bitcoin --log_level=all --run_test=argsman_tests
```

Then I ran them al
...

💬 sedited commented on pull request "Add util::Expected (std::expected)":
(https://github.com/bitcoin/bitcoin/pull/34006#issuecomment-3612743633)
Concept ACK

💬 Sjors commented on pull request "Add util::Expected (std::expected)":
(https://github.com/bitcoin/bitcoin/pull/34006#issuecomment-3612759553)
Concept ACK

`FetchBlock()` is a good usage example.

💬 fanquake commented on pull request "guix: use GCC 14.3.0 over 13.3.0":
(https://github.com/bitcoin/bitcoin/pull/33775#issuecomment-3612808384)
Minified a little more (https://github.com/fanquake/bitcoin/tree/repro_33775_minimal):
```cpp
#include <filesystem>
#include <string>
#include <vector>

#include <cstring> // Unused, but removing this makes it determinstic

namespace fs {

using namespace std::filesystem;

// Dummy class. Using "path = std::filesystem::path" makes it deterministic.
class path : public std::filesystem::path
{
public:
using std::filesystem::path::path;

path(std::filesystem::path path) :
...

💬 fanquake commented on issue "FUZZ=psbt in musig2, runs into uninitialized read":
(https://github.com/bitcoin/bitcoin/issues/33999#issuecomment-3612862383)
https://issues.oss-fuzz.com/issues/465913232.

💬 instagibbs commented on issue "RFC: when to drop testnet3":
(https://github.com/bitcoin/bitcoin/issues/31975#issuecomment-3612877414)
I'll steelman an even stronger suggestion: Don't drop testnet3?

What is the maintenance burden involved in keeping it around?

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2589593339)
Applied locally.

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2589601292)
Applied locally.

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2589631104)
I do not think so, but I cannot be 100% sure. What to do? Leave it as it is and revisit later if it ever becomes a problem?

💬 mzumsande commented on issue "RFC: when to drop testnet3":
(https://github.com/bitcoin/bitcoin/issues/31975#issuecomment-3612930147)
> I'll steelman an even stronger suggestion: Don't drop testnet3?
>
> What is the maintenance burden involved in keeping it around?

I thought the hope was that it would deter scammers/shitcoiners from trying to establish a market for testnet coins when Bitcoin Core doesn't shy away from dropping support for the network.

💬 maflcko commented on pull request "refactor: replace manual promise with SyncWithValidationInterfaceQueue":
(https://github.com/bitcoin/bitcoin/pull/33962#discussion_r2589784708)
Please just push the prior exact commit hash.

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2589789136)
Marking this as resolved as work on it continues at https://github.com/bitcoin/bitcoin/pull/33954