💬 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())) {
```

💬 maflcko commented on pull request "Add `SigVersion` parameter to `IsOpSuccess()`":
(https://github.com/bitcoin/bitcoin/pull/29265#discussion_r1457915420)
Using `default` in a switch will disable compiler warnings and make code more brittle. See the developer notes on how to handle this.

💬 maflcko commented on pull request "refactor: Fix prevector iterator concept issues":
(https://github.com/bitcoin/bitcoin/pull/29275#issuecomment-1899114359)
Fixed libc++ CIs

💬 1440000bytes commented on issue "Witness scripts being abused to bypass datacarriersize limit (CVE-2023-50428)":
(https://github.com/bitcoin/bitcoin/issues/29187#issuecomment-1899130684)
@wizkid057 I searched for the word "exploit" and found you have used it several times in your messages, Which of these opcodes is not working as implemented or [documented](https://github.com/ChrisCho-H/bitcoin/blob/0bf55345764d22c5653c0a374647aa5582a186a2/src/script/script.h)?

If you want to add another config option for certain transaction that are considered "spam" by a few users, why not open a pull request that allows users to do it without changing defaults?

I have suggested 2 approa
...

💬 Christewart commented on pull request "Add `SigVersion` parameter to `IsOpSuccess()`":
(https://github.com/bitcoin/bitcoin/pull/29265#discussion_r1457935010)
I attempted to fix this in c1fe7109fc989c263c19696229c73c5982ff19e7

Is that what things should look like? From Pieter's original comment, it seemed like asserting false in the case of non tapscript sig versions are was desirable.

https://github.com/bitcoin/bitcoin/pull/29265#discussion_r1457862849