💬 ryanofsky commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1457662881)
In commit "[refactor] De-globalize ValidationSignals" (6ffa572edd9693a3217eb201b9d147b44c5b4eb5)

Mentioned earlier, but you should drop the new parameter here and just call `wallet->chain().waitForNotificationsIfTipChanged({})` instead of `SyncWithValidationInterfaceQueue`, for the same reasons

💬 ryanofsky commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1457639359)
In commit "scripted-diff: Rename MainSignals to ValidationSignals" (0fd494c4a856d7aefd4fd94a4d5f18ab5e59c2a2)

Not important, but I think it would be a little nicer if scripted diff was last commit instead of first commit, because it's hard to tell whether renames are good or bad when the code is in flux, and better when you can see that names match state of the final code. Moving the scripted diff would also reduce churn within the PR so commit diffs are smaller and lines like this (which are
...

💬 ryanofsky commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1457671335)
In commit "[refactor] De-globalize ValidationSignals" (6ffa572edd9693a3217eb201b9d147b44c5b4eb5)

Note: This is ugly, but #24230 replaces it with `m_chain->attachChain()`. Other changes to indexing code in this PR, which also don't look great, have similar replacements.

💬 ryanofsky commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1457667372)
In commit "[refactor] De-globalize ValidationSignals" (6ffa572edd9693a3217eb201b9d147b44c5b4eb5)

Would suggest organizing the parameters {validation_signals, interrupt, chainman_opts, blockman_opts} so ChainstateManager dependencies are first and ChainstateManager options are last. Also so dependencies and options are both ordered from high level to low level.

💬 ryanofsky commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1457656786)
In commit "[refactor] De-globalize ValidationSignals" (6ffa572edd9693a3217eb201b9d147b44c5b4eb5)

These wallet tests should be calling `wallet.chain().waitForNotificationsIfTipChanged({})` instead of `test_setup->m_node.validation_signals->SyncWithValidationInterfaceQueue`. This would make them more realistic and less tied to test and node internals.

💬 ryanofsky commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1457683117)
In commit "[refactor] De-globalize ValidationSignals" (6ffa572edd9693a3217eb201b9d147b44c5b4eb5)

Note: At some point we should see if it makes sense to deduplicate this function with `AppInit` and `BasicTestingSetup::BasicTestingSetup` since all three functions are doing very similar things.

💬 ryanofsky commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1457687097)
In commit "[refactor] De-globalize ValidationSignals" (6ffa572edd9693a3217eb201b9d147b44c5b4eb5)

Would again suggest making make signals parameter first. `signals` is the hard dependency and input, `notifications` is the option and output.

💬 ryanofsky commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1457699474)
In commit "[refactor] De-globalize ValidationSignals" (6ffa572edd9693a3217eb201b9d147b44c5b4eb5)

`m_signals` declaration should be moved directly above `m_proxy` declaration. IMO it is a bad practice to mix up method declarations and variable declarations in a class definition. The most important thing you can know about a class is what it information contains and what it represents, and that is difficult to do if you can't easily see a list of its member variables.

Additionally, all membe
...

💬 Christewart commented on pull request "Add `SigVersion` parameter to `IsOpSuccess()`":
(https://github.com/bitcoin/bitcoin/pull/29265#discussion_r1457856682)
I don't see how this is better with `SigVersion`, perhaps I'm missing some pattern matching features in c++ that would enforce exhaustiveness checks?

💬 Christewart commented on pull request "Add `SigVersion` parameter to `IsOpSuccess()`":
(https://github.com/bitcoin/bitcoin/pull/29265#issuecomment-1899032924)
> I think you'd want to pass SigVersion rather than leaf_version. The SigVersion defines what script/signature rules are active, while the leaf_version is one of several ways of encoding that information in scripts. It also answers your comment: we don't care about unknown leaf_versions because they will never occur (under an unknown leaf version, no script execution happens at all).

done in https://github.com/bitcoin/bitcoin/pull/29265/commits/5600629cdb2530699b7da453f398f4bb259c8b04

💬 sipa commented on pull request "Add `SigVersion` parameter to `IsOpSuccess()`":
(https://github.com/bitcoin/bitcoin/pull/29265#discussion_r1457858379)
There simply wouldn't *exist* a SigVersion for unknown leaf version.

💬 ryanofsky commented on issue "build: multiprocess compile failure on macOS":
(https://github.com/bitcoin/bitcoin/issues/29248#issuecomment-1899038653)
Thanks for the reminder. Will try to implement a fix for this today!

💬 Christewart commented on pull request "Add `SigVersion` parameter to `IsOpSuccess()`":
(https://github.com/bitcoin/bitcoin/pull/29265#discussion_r1457862419)
So when you say that I assume you want this

```c++
bool IsOpSuccess(const opcodetype& opcode, SigVersion sigversion)
{
if (sigversion == SigVersion::TAPSCRIPT) {
return opcode == 80 || opcode == 98 || (opcode >= 126 && opcode <= 129) ||
(opcode >= 131 && opcode <= 134) || (opcode >= 137 && opcode <= 138) ||
(opcode >= 141 && opcode <= 142) || (opcode >= 149 && opcode <= 153) ||
(opcode >= 187 && opcode <= 254);
}
```

However this g
...

💬 sipa commented on pull request "Add `SigVersion` parameter to `IsOpSuccess()`":
(https://github.com/bitcoin/bitcoin/pull/29265#discussion_r1457862849)
I would suggest a switch case, over all possible SigVersions. The non-taproot ones can just contain an assert false, as `IsOpSuccess` shouldn't be called for anything non-taproot.

Alternatively, there could also be separate `IsOpSuccessTapscript` and `IsOpSuccessFooscript` functions, which get called in the respective branches.

💬 Christewart commented on pull request "Add `SigVersion` parameter to `IsOpSuccess()`":
(https://github.com/bitcoin/bitcoin/pull/29265#discussion_r1457864400)
Ok, both of these make sense to me. I'll do the switch implementation for now.

💬 Christewart commented on pull request "Add `SigVersion` parameter to `IsOpSuccess()`":
(https://github.com/bitcoin/bitcoin/pull/29265#discussion_r1457881873)
This triggers the linter

>A new circular dependency in the form of "script/interpreter -> script/script -> script/interpreter" appears to have been introduced.

Is there an easy way to get around this? The include is for `SigVersion`

🤔 murchandamus reviewed a pull request: "Add max_tx_weight to transaction funding options"
(https://github.com/bitcoin/bitcoin/pull/29264#pullrequestreview-1830384740)
Concept ACK, got some suggestions how you can give Knapsack a bit of a fighting chance, although we should get rid of Knapsack instead of improving it.

💬 murchandamus commented on pull request "Add max_tx_weight to transaction funding options":
(https://github.com/bitcoin/bitcoin/pull/29264#discussion_r1457887218)
I would suggest that you pass the `max_weight` as an additional parameter to `ApproximateBestSubset(…)` and keep track of the current weight (e.g. `nTotalWeight`) in parallel to `nTotal` and only designate a new `nBest` and `vfBest` if it’s not just better but also a permitted weight:

```diff
- if (nTotal < nBest)
+ if (nTotal < nBest && nTotalWeight <= max_weight)
```

for extra credit you could also deselect not just
...

💬 murchandamus commented on pull request "Add max_tx_weight to transaction funding options":
(https://github.com/bitcoin/bitcoin/pull/29264#discussion_r1457876258)
Does 0 even ever make sense?

💬 murchandamus commented on pull request "Add max_tx_weight to transaction funding options":
(https://github.com/bitcoin/bitcoin/pull/29264#discussion_r1457871015)
In line 331:

```diff
- } else if (!lowest_larger || group.GetSelectionAmount() < lowest_larger->GetSelectionAmount()) {
+ } else if (group.m_weight <= max_weight && (!lowest_larger || group.GetSelectionAmount() < lowest_larger->GetSelectionAmount())) {
```