🤔 sipa reviewed a pull request: "miniscript refactor: Remove unique_ptr-indirection"
(https://github.com/bitcoin/bitcoin/pull/31713#pullrequestreview-3553094316)
reACK d782bffc5461f7361ae410963466ae1577ef63df

💬 maflcko commented on pull request "Add util::Expected (std::expected)":
(https://github.com/bitcoin/bitcoin/pull/34006#discussion_r2599407230)
It'll be used in the kernel, soon. So I'd rather avoid the churn. In any case, it will likely remain empty, and behavior should all be the same. So I think it doesn't matter and my preference would be to just keep this as-is for now.

💬 maflcko commented on pull request "Add util::Expected (std::expected)":
(https://github.com/bitcoin/bitcoin/pull/34006#discussion_r2599410546)
ok, i'll try to fixup the second commit tomorrow, or in a follow-up, whichever happens earlier.

🤔 hebasto reviewed a pull request: "guix: reduce allowed exported symbols"
(https://github.com/bitcoin/bitcoin/pull/33950#pullrequestreview-3553159236)
> FWIW i cherry-picked commit [7b90b4f](https://github.com/bitcoin/bitcoin/commit/7b90b4f5bb10e2156709b07e3996f867e2421232) on top of [b354d1c](https://github.com/bitcoin/bitcoin/commit/b354d1ce5c47997aa2f93910e05c0f8daa8736eb) (the commit before #33181). A full guix build finished succesfully. So it must have been something earlier even!

The `_fini` and `_init` symbols have no longer been exported since commit b5fc6d46a3854c18f6e8dfc89540d24ef778caa2 in https://github.com/bitcoin/bitcoin/pull/
...

💬 ryanofsky commented on issue "Memory leak when using IPC mining interface":
(https://github.com/bitcoin/bitcoin/issues/33940#issuecomment-3628241016)
re: https://github.com/bitcoin/bitcoin/issues/33940#issuecomment-3611006053

>* `git clone https://github.com/stratum-mining/sv2-apps -b v0.1.0`

Thanks for the instructions. I've been experimenting with the v0.1.0 pool_sv2 app on regtest with a python script to generate transactions & connect blocks quickly to make a memory leak happen without needing to wait a long time.

So far, behavior has been mostly as expected, where generating lots of transactions without connecting new blocks causes bl
...

💬 Sjors commented on issue "Memory leak when using IPC mining interface":
(https://github.com/bitcoin/bitcoin/issues/33940#issuecomment-3628281213)
> if the python client connected blocks too quickly, the mining client seemed to fall behind and only release templates after a delay

There's a hardcoded 10(?) second grace period where the Template Provider (both implementations) hold on to templates after a new tip is connected. It's there for two reasons:

1. Trying to relay the block anyway (requires further node changes, since we currently honour the first-seen rule even when that means "our" block loses)
2. Giving pools a chance to verify
...

💬 theuni commented on issue "Net split meta issue":
(https://github.com/bitcoin/bitcoin/issues/33958#issuecomment-3628282838)

> In particular, I think good goals to have here are (not in priority order):
>
> 1. simplify the codebase so it's easier to understand and update
>
> 2. improve robustness of the codebase, so that if there are bugs, they have less impact
>
> 3. make the code higher performance, so that we can have more peers, and/or run on cheaper hardware
>

Goal #1 is a logical separation of concerns. Today, the many of interactions between `CConnman` and `PeerManager` are subtle and (I'd gue
...

👍 ryanofsky approved a pull request: "doc: document capnproto and libmultiprocess deps in 29.x"
(https://github.com/bitcoin/bitcoin/pull/33623#pullrequestreview-3553439261)
Code review ACK 2cf352fd8e6a77003e38d954b6c879b20d4b960a. Thanks for making all these changes and for opening the fix originally.

👍 ryanofsky approved a pull request: "mining: getCoinbase() returns struct instead of raw tx"
(https://github.com/bitcoin/bitcoin/pull/33819#pullrequestreview-3553451082)
Code review ACK 1bd6cae51fedeff62755e03ddcb9346b868f6d83. Just rebased with no changes, to avoid trivial merge conflict.

💬 ryanofsky commented on pull request "test: interface_ipc.py minor fixes and cleanup":
(https://github.com/bitcoin/bitcoin/pull/34003#discussion_r2599729002)
re: https://github.com/bitcoin/bitcoin/pull/34003#discussion_r2598858386

> In [fac12f7](https://github.com/bitcoin/bitcoin/commit/fac12f7cf9ebc0b906bbb20ae126cf7e7552fdbb) _test: fix interface_ipc.py template destruction_: similar to my comment above, this only shows that `waitNext()` returns immediately if fees have gone up before the call, but the original intent was to show that a fee increase causes the wait to be interrupted.

Makes sense. Again there should be no change of behavior in
...

💬 ryanofsky commented on pull request "test: interface_ipc.py minor fixes and cleanup":
(https://github.com/bitcoin/bitcoin/pull/34003#discussion_r2599716362)
re: https://github.com/bitcoin/bitcoin/pull/34003#discussion_r2598844209

> I think the original intention here was to demonstrate that `generate` interrupts an ongoing `waitNext()`.
>
> Now it only shows that `waitNext()` returns immediately if the tip has changes before calling. Though perhaps it always did that by accident.

Yes behavior should be unchanged. Original code was:

```python
waitnext = template.result.waitNext(ctx, waitoptions)
self.generate(self.nodes[0], 1)
template
...

💬 Sjors commented on pull request "validation: Remove min_pow_checked arg in ProcessNewBlockHeaders":
(https://github.com/bitcoin/bitcoin/pull/34022#issuecomment-3628534056)
This was intentional, as explained in the commit message of ed6cddd98e32263fc116a4380af6d66da20da990 (part of #25717):

> This commit adds a new argument to `AcceptBlockHeader()` so that we can ensure that all call-sites which might cause a new header to be accepted into memory have to grapple with the question of whether the header is safe to accept, or needs further validation.

Not sure if we need to keep it forever.

cc @sdaftuar

💬 Sjors commented on pull request "validation: Remove min_pow_checked arg in ProcessNewBlockHeaders":
(https://github.com/bitcoin/bitcoin/pull/34022#discussion_r2599761636)
Maybe something loosely based on the original commit message:

> all call-sites which might cause a new header to be accepted into memory have to grapple with the question of whether the header is safe to accept, or needs further validation

🤔 mzumsande reviewed a pull request: "Broadcast own transactions only via short-lived Tor or I2P connections"
(https://github.com/bitcoin/bitcoin/pull/29415#pullrequestreview-3553664703)
re-ACK https://github.com/bitcoin/bitcoin/commit/f391296b17a755153960e0afc736df37d1d5fb1e

💬 instagibbs commented on pull request "Replace cluster linearization algorithm with SFL":
(https://github.com/bitcoin/bitcoin/pull/32545#discussion_r2590419145)
do you mean pick a chunk randomly, then observer the dependency/dependee list, and pick the corresponding lowers/highest feerate one for activation?

💬 instagibbs commented on pull request "Replace cluster linearization algorithm with SFL":
(https://github.com/bitcoin/bitcoin/pull/32545#discussion_r2590509887)
```Suggestion
// If we started from a topological input, the resulting feerate diagram cannot be worse or incomparable.
```

💬 instagibbs commented on pull request "Replace cluster linearization algorithm with SFL":
(https://github.com/bitcoin/bitcoin/pull/32545#discussion_r2593658728)
```Suggestion
// De-activate it otherwise.
```

💬 instagibbs commented on pull request "Replace cluster linearization algorithm with SFL":
(https://github.com/bitcoin/bitcoin/pull/32545#discussion_r2590318854)
```Suggestion
* The collection of all spanning trees for the entire cluster form a spanning forest. In the extreme each
```

💬 instagibbs commented on pull request "Replace cluster linearization algorithm with SFL":
(https://github.com/bitcoin/bitcoin/pull/32545#discussion_r2590522916)
85cff48b782538ed5a1c980015dd01cb32986b66

straggling AncestorCandidateFinder reference

💬 instagibbs commented on pull request "Replace cluster linearization algorithm with SFL":
(https://github.com/bitcoin/bitcoin/pull/32545#discussion_r2590327658)
> or equal

Need to understand why, delving seems to suggest the more "natural" choice https://delvingbitcoin.org/t/spanning-forest-cluster-linearization/1419 "If there is an inactive dependency, where the parent and child are in distinct chunks, and the child chunk has higher feerate than the parent chunk, make the dependency active, merging the two chunks."

💬 sipa commented on pull request "Replace cluster linearization algorithm with SFL":
(https://github.com/bitcoin/bitcoin/pull/32545#discussion_r2599895617)
If you don't merge along equal-feerate dependencies, you can end up in a situation with two equal-feerate "chunks" that depend on each other.

E.g. the AC and BD "chunks" in:

```mermaid
graph BT;
A["A: 1/1"];
B["B: 1/1"];
C["C: 1/1"];
D["D: 1/1"];
C --> A;
C -.-> B;
D -.-> A;
D --> B;
```

The solution is an initial phase where you don't care about splitting equal-feerate parts from one another (this PR), and then a secondary phase where you split up chunks in their minimal comp
...