💬 sipa commented on pull request "Replace cluster linearization algorithm with SFL":
(https://github.com/bitcoin/bitcoin/pull/32545#discussion_r2607125395)
It's not, but it's pretty subtle.

You could have two equal-feerate chunks that depend on each other in one way but not the other one. If so, it doesn't matter whether they're merged or not.

Worth elaborating on?

💬 sipa commented on pull request "Replace cluster linearization algorithm with SFL":
(https://github.com/bitcoin/bitcoin/pull/32545#discussion_r2607141755)
I have pushed another updated which drops the "maximum feerate difference" explanation entirely. I think encapsulating all of that within "merge upwards" and "merge downwards" rules is more concrete.

To be clear, the "uniformly random" aspect is something that doesn't exist in the initial "[add class implementing SFL state](https://github.com/bitcoin/bitcoin/pull/32545/commits/a21a2451c60918944ebe33eb71725872b7655821)" PR, it's added in a slightly later commit. The reword commit is at the end
...

💬 sipa commented on pull request "Replace cluster linearization algorithm with SFL":
(https://github.com/bitcoin/bitcoin/pull/32545#discussion_r2607142420)
Thanks, fixed!

💬 sipa commented on pull request "Replace cluster linearization algorithm with SFL":
(https://github.com/bitcoin/bitcoin/pull/32545#discussion_r2607142876)
Fixed.

💬 instagibbs commented on pull request "Replace cluster linearization algorithm with SFL":
(https://github.com/bitcoin/bitcoin/pull/32545#discussion_r2607162176)
ok then I'm reading it correctly and agree to drop the language (I couldn't tell if it was new info or a restatement of prior)

💬 instagibbs commented on pull request "Replace cluster linearization algorithm with SFL":
(https://github.com/bitcoin/bitcoin/pull/32545#discussion_r2607167947)
actually not sure what I meant here on second read, ignore

💬 romanz commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2607170399)
Modified `blockstorage: allow reading partial block data from storage` to fix the issue above:

<details>

<summary> range-diff </summary>

```
$ git range-diff origin/master 2251ac1 8b41708

1: 1be1c165b3 = 1: 1be1c165b3 blockstorage: return an error code from `ReadRawBlock()`
2: 0fa70db617 ! 2: 52ac60d141 blockstorage: allow reading partial block data from storage
@@ src/node/blockstorage.h: public:
bool ReadBlockUndo(CBlockUndo& blockundo, const CBlockIndex& inde
...

👍 hodlinator approved a pull request: "util: Add some more Unexpected and Expected methods"
(https://github.com/bitcoin/bitcoin/pull/34032#pullrequestreview-3561476416)
ACK fabef9bbef255796f28c04c5b478df9544c749eb

(Not yet fully at ease with ampersands *after* the method parameters such as in the last two commits, example: `constexpr E&& error() &&`).

💬 hodlinator commented on pull request "util: Add some more Unexpected and Expected methods":
(https://github.com/bitcoin/bitcoin/pull/34032#discussion_r2607103489)
nit: Could rename `err` -> `m_err` for consistency with `Expected::m_data`.

💬 hodlinator commented on pull request "util: Add some more Unexpected and Expected methods":
(https://github.com/bitcoin/bitcoin/pull/34032#discussion_r2605806608)
nit: Could use BOOST_CHECK_EQUAL when applicable:

```suggestion
BOOST_CHECK_EQUAL(e.value().x, 42);
```

💬 hodlinator commented on pull request "util: Add some more Unexpected and Expected methods":
(https://github.com/bitcoin/bitcoin/pull/34032#discussion_r2607135191)
~nit: Could use `optional` instead of `monostate`?~ Ah, I see that was already discussed https://github.com/bitcoin/bitcoin/pull/34032#issuecomment-3632418418.

<details><summary>Diff</summary>

```diff
--- a/src/util/expected.h
+++ b/src/util/expected.h
@@ -10,6 +10,7 @@

#include <cassert>
#include <exception>
+#include <optional>
#include <utility>
#include <variant>

@@ -119,16 +120,16 @@ template <class E>
class Expected<void, E>
{
private:
- std::variant<std:
...

💬 Crypt-iQ commented on pull request "fuzz: Add fuzz target for block index tree and related validation events":
(https://github.com/bitcoin/bitcoin/pull/31533#discussion_r2607189790)
I was pretty confused about the second call to `InvalidChainFound` when reading the code, so I think it makes sense to PR and remove. It seems to have been introduced in https://github.com/bitcoin/bitcoin/pull/4148 and only affects the warnings in `CheckForkWarningConditions`.

Since `vpindexToConnect.front()` must descend (or be equal to) from the `CBlockIndex*` passed to `InvalidBlockFound` and all descendants of this `CBlockIndex*` will have been marked as invalid in `SetBlockFailureFlags`,
...

💬 ryanofsky commented on pull request "kernel: Improve logging API":
(https://github.com/bitcoin/bitcoin/pull/33847#discussion_r2607212025)
I see comment has been edited, but just to respond to the example, since it is illustrative:

>```c++
>opts = btck_context_options_create();
>logger = btck_logging_connection_create();
>btck_context_options_set_logger(opts, logger);
>ctx = btck_context_create(opts);
>btck_logging_connection_destroy(loger);
>```

I'd want to resolve this by adding a `int use_count` field to the `LoggingConnection` struct and making the `btck_logging_connection_destroy` call fail if the use_count is not
...

💬 romanz commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2607215490)
Fixed in 8b417087ae

💬 romanz commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2607216804)
Dropped in 1be1c165b3

💬 romanz commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2607218695)
Thanks - updated in 8b417087ae

💬 romanz commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2607222862)
Changed in 8b417087ae.

💬 romanz commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2607223451)
Fixed in 8b417087ae

💬 romanz commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2607225790)
Fixed in 1be1c165b3 & 52ac60d141

💬 instagibbs commented on pull request "Replace cluster linearization algorithm with SFL":
(https://github.com/bitcoin/bitcoin/pull/32545#discussion_r2607229744)
ok, so I just "forgot" again that "or equal" means you can clearly have same feerate chunks that can be merged, hence not topological in the SFL state sense

💬 0xB10C commented on pull request "test: address self-announcement":
(https://github.com/bitcoin/bitcoin/pull/34039#discussion_r2607241581)
Done! Wasn't aware of `bumpmocktime`, thanks. Cleaned up use of mocktime a bit.