💬 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

📝 ryanofsky opened a pull request: "depends: Update libmultiprocess library to fix C++20 macos build error"
(https://github.com/bitcoin/bitcoin/pull/29276)
Fixes #29248

The std::result_of type was removed in c++20, but was being referenced in some old, unused code in the library. The issue was fixed in:

- https://github.com/chaincodelabs/libmultiprocess/pull/91

This update also includes other recent libmultiprocess changes to improve C++20 support and fix build issues:

- https://github.com/chaincodelabs/libmultiprocess/pull/89
- https://github.com/chaincodelabs/libmultiprocess/pull/90

💬 ryanofsky commented on issue "build: multiprocess compile failure on macOS":
(https://github.com/bitcoin/bitcoin/issues/29248#issuecomment-1899144292)
This should be fixed by https://github.com/chaincodelabs/libmultiprocess/pull/91 and https://github.com/bitcoin/bitcoin/pull/29276.

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

You already know the answer to this, but are asking as if it's some kind of "gotcha" here. It is not.

Whether or not stuffing arbitrary data between valid opcodes is c
...

👍 ryanofsky approved a pull request: "Avoid returning references to mutex guarded members"
(https://github.com/bitcoin/bitcoin/pull/28774#pullrequestreview-1830531507)
Code review ACK 32a9f13cb805ecf6aebb5cf4e5d92b8a47c4548b. This seems like a potentially real race condition, and the fix here is pretty simple.

I do think the PR title and description are confusing and probably causing this PR to get overlooked.

Would suggest replacing PR title and description with the title and description from the 32a9f13cb805ecf6aebb5cf4e5d92b8a47c4548b commit message which are much clearer. To make the PR comment history legible, you could include the original descript
...