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