🤔 achow101 reviewed a pull request: "wallet, rpc: add v3 transaction creation and wallet support"
(https://github.com/bitcoin/bitcoin/pull/32896#pullrequestreview-3085782791)
I think `sendall` is missing the weight check if the transaction is truc.
(https://github.com/bitcoin/bitcoin/pull/32896#pullrequestreview-3085782791)
I think `sendall` is missing the weight check if the transaction is truc.
💬 achow101 commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2252602918)
In 0256da5083b6a430993594b227fbae4e005e6024 "wallet: mark unconfirmed v3 siblings as mempool conflicts"
nit: This code is unchanged, renaming is not necessary here.
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2252602918)
In 0256da5083b6a430993594b227fbae4e005e6024 "wallet: mark unconfirmed v3 siblings as mempool conflicts"
nit: This code is unchanged, renaming is not necessary here.
💬 achow101 commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2252630081)
In 894a05477e23bddb7385467d0dc07739d8557b6c "rpc: Support version 3 transaction creation"
nit: `util::ToString` is unnecessary if `%d` is used as the format specifier.
```suggestion
throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Invalid parameter, version out of range(%d~%d)", TX_MIN_STANDARD_VERSION, TX_MAX_STANDARD_VERSION));
```
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2252630081)
In 894a05477e23bddb7385467d0dc07739d8557b6c "rpc: Support version 3 transaction creation"
nit: `util::ToString` is unnecessary if `%d` is used as the format specifier.
```suggestion
throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Invalid parameter, version out of range(%d~%d)", TX_MIN_STANDARD_VERSION, TX_MAX_STANDARD_VERSION));
```
💬 achow101 commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2252649623)
In 09c9f828250e3840d7557374ff568204305e2679 "test: add truc wallet tests"
This seems unnecessary as `setup_network` will already connect our nodes, and it doesn't look like this test does any disconnections or requires a particular node topography.
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2252649623)
In 09c9f828250e3840d7557374ff568204305e2679 "test: add truc wallet tests"
This seems unnecessary as `setup_network` will already connect our nodes, and it doesn't look like this test does any disconnections or requires a particular node topography.
💬 achow101 commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2252616177)
In 8d085d4c2ca451af75a9adfcaebb8cc0ccb3d86c "wallet: limit v3 tx weight in coin selection"
This check is redundant since `FundTransaction` is being called and it already is setting the weight appropriately.
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2252616177)
In 8d085d4c2ca451af75a9adfcaebb8cc0ccb3d86c "wallet: limit v3 tx weight in coin selection"
This check is redundant since `FundTransaction` is being called and it already is setting the weight appropriately.
💬 achow101 commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2252658444)
In 09c9f828250e3840d7557374ff568204305e2679 "test: add truc wallet tests"
s/unavailable/available
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2252658444)
In 09c9f828250e3840d7557374ff568204305e2679 "test: add truc wallet tests"
s/unavailable/available
💬 achow101 commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2252652028)
In 09c9f828250e3840d7557374ff568204305e2679 "test: add truc wallet tests"
It looks like we end up having just one wallet per node, and the tests only really care about the wallet separation, not that they are on different nodes. So this should be just `self.num_nodes = 1` with multiple wallets on that single node.
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2252652028)
In 09c9f828250e3840d7557374ff568204305e2679 "test: add truc wallet tests"
It looks like we end up having just one wallet per node, and the tests only really care about the wallet separation, not that they are on different nodes. So this should be just `self.num_nodes = 1` with multiple wallets on that single node.
💬 achow101 commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2252663859)
In 09c9f828250e3840d7557374ff568204305e2679 "test: add truc wallet tests"
Since all of the test cases do a `self.generate`, that could be added here as well before `func`.
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2252663859)
In 09c9f828250e3840d7557374ff568204305e2679 "test: add truc wallet tests"
Since all of the test cases do a `self.generate`, that could be added here as well before `func`.
💬 achow101 commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2252669590)
In 09c9f828250e3840d7557374ff568204305e2679 "test: add truc wallet tests"
I don't see how this test is materially different from `truc_tx_with_conflicting_sibling`. Based on the naming, I assume this was added in response to https://github.com/bitcoin/bitcoin/pull/32896/commits/09c9f828250e3840d7557374ff568204305e2679#r2211372152? However, this test isn't testing anything with a change output since the parent tx is from charlie.
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2252669590)
In 09c9f828250e3840d7557374ff568204305e2679 "test: add truc wallet tests"
I don't see how this test is materially different from `truc_tx_with_conflicting_sibling`. Based on the naming, I assume this was added in response to https://github.com/bitcoin/bitcoin/pull/32896/commits/09c9f828250e3840d7557374ff568204305e2679#r2211372152? However, this test isn't testing anything with a change output since the parent tx is from charlie.
💬 achow101 commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2252660514)
In 09c9f828250e3840d7557374ff568204305e2679 "test: add truc wallet tests"
The `getrawmempool` can be avoided:
```suggestion
parent_txid = self.send_tx(self.charlie, inputs, outputs, 3)
```
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2252660514)
In 09c9f828250e3840d7557374ff568204305e2679 "test: add truc wallet tests"
The `getrawmempool` can be avoided:
```suggestion
parent_txid = self.send_tx(self.charlie, inputs, outputs, 3)
```
💬 Crypt-iQ commented on pull request "validation: detect witness stripping early on":
(https://github.com/bitcoin/bitcoin/pull/33105#discussion_r2252747448)
I think P2SH-wrapped taproot programs are non-standard according to BIP341 text (says they remain unencumbered) and [here](https://github.com/bitcoin/bitcoin/blob/d1b583181dcc31613cd586d63329cb4c4a586972/src/script/interpreter.cpp#L1949)?
(https://github.com/bitcoin/bitcoin/pull/33105#discussion_r2252747448)
I think P2SH-wrapped taproot programs are non-standard according to BIP341 text (says they remain unencumbered) and [here](https://github.com/bitcoin/bitcoin/blob/d1b583181dcc31613cd586d63329cb4c4a586972/src/script/interpreter.cpp#L1949)?
💬 l0rinc commented on pull request "index: Fix missing case in the comment in NextSyncBlock()":
(https://github.com/bitcoin/bitcoin/pull/32875#discussion_r2252779057)
seems you've found a code that isn't covered by tests - could you add a test case which showcases the need for this addition (such that if we revert this code the test fails)?
(https://github.com/bitcoin/bitcoin/pull/32875#discussion_r2252779057)
seems you've found a code that isn't covered by tests - could you add a test case which showcases the need for this addition (such that if we revert this code the test fails)?
👍 l0rinc approved a pull request: "index: remove unnecessary locater cleaning in BaseIndex::Init()"
(https://github.com/bitcoin/bitcoin/pull/32882#pullrequestreview-3086116216)
ACK 1f678d832b1196ca7d7c2ff7e70d2172ae8c90b6
Recreated and retested it locally. It's a similar change to https://github.com/bitcoin/bitcoin/pull/33042, we have a few more places which should be cleanup up like this.
I think this already makes the code slightly simpler, but alternatively, you could return an optional and not rely output params and nullness
<details>
<summary>Details</summary>
```patch
diff --git a/src/index/base.cpp b/src/index/base.cpp
index fdd0e0d8af..0408021fd5
...
(https://github.com/bitcoin/bitcoin/pull/32882#pullrequestreview-3086116216)
ACK 1f678d832b1196ca7d7c2ff7e70d2172ae8c90b6
Recreated and retested it locally. It's a similar change to https://github.com/bitcoin/bitcoin/pull/33042, we have a few more places which should be cleanup up like this.
I think this already makes the code slightly simpler, but alternatively, you could return an optional and not rely output params and nullness
<details>
<summary>Details</summary>
```patch
diff --git a/src/index/base.cpp b/src/index/base.cpp
index fdd0e0d8af..0408021fd5
...
💬 murchandamus commented on pull request "test: add assertions to SRD max weight test":
(https://github.com/bitcoin/bitcoin/pull/33058#issuecomment-3152836001)
> This test case has a number of invaluable utxos and some valuable ones. Would it not be better to show that after the min-heap is added, that the implementation has a bias towards selecting valuable utxos? I think these types of implementation details would be valuable to test.
[…]
> However, to test the above behavior, I think it would be difficult using a non-deterministic test.
That’s exactly what the test shows: in order to stay under the `max_selection_weight`, BnB has to select
...
(https://github.com/bitcoin/bitcoin/pull/33058#issuecomment-3152836001)
> This test case has a number of invaluable utxos and some valuable ones. Would it not be better to show that after the min-heap is added, that the implementation has a bias towards selecting valuable utxos? I think these types of implementation details would be valuable to test.
[…]
> However, to test the above behavior, I think it would be difficult using a non-deterministic test.
That’s exactly what the test shows: in order to stay under the `max_selection_weight`, BnB has to select
...
💬 murchandamus commented on pull request "coinselection: Optimize BnB exploration":
(https://github.com/bitcoin/bitcoin/pull/32150#issuecomment-3152850345)
Fixed the merge conflict with #33060.
(https://github.com/bitcoin/bitcoin/pull/32150#issuecomment-3152850345)
Fixed the merge conflict with #33060.
💬 ryanofsky commented on pull request "test,refactor: extract script template helpers & widen sigop count coverage":
(https://github.com/bitcoin/bitcoin/pull/32729#discussion_r2252835441)
In commit "refactor: rename `GetSigOpCount` to `GetLegacySigOpCount`" (06cb252c8285746737e3385886da2efd1ab41f5a)
I like getting rid of this overload, but I found the code here very confusing before this change, and I think renaming this to `GetLegacySigOpCount` makes the confusion worse because this method is not acting like a legacy function when it's called with fAccurate = true. At first I though you might have been using legacy here to mean "pre-segwit" which is different than the way the
...
(https://github.com/bitcoin/bitcoin/pull/32729#discussion_r2252835441)
In commit "refactor: rename `GetSigOpCount` to `GetLegacySigOpCount`" (06cb252c8285746737e3385886da2efd1ab41f5a)
I like getting rid of this overload, but I found the code here very confusing before this change, and I think renaming this to `GetLegacySigOpCount` makes the confusion worse because this method is not acting like a legacy function when it's called with fAccurate = true. At first I though you might have been using legacy here to mean "pre-segwit" which is different than the way the
...
🤔 l0rinc requested changes to a pull request: "truc: optimize the in package relation calculation"
(https://github.com/bitcoin/bitcoin/pull/33062#pullrequestreview-3086125986)
My understanding is that this isn't a bottleneck, optimizing it without a benchmark or an easily reproducible usecase seems like it's a solution begging for a problem - I speak from personal experience, that's how I started as well :)
I left a few comments while reviewing anyway, but without a concept ack on the idea itself from those who are more familiar with it, it likely won't get merged.
(https://github.com/bitcoin/bitcoin/pull/33062#pullrequestreview-3086125986)
My understanding is that this isn't a bottleneck, optimizing it without a benchmark or an easily reproducible usecase seems like it's a solution begging for a problem - I speak from personal experience, that's how I started as well :)
I left a few comments while reviewing anyway, but without a concept ack on the idea itself from those who are more familiar with it, it likely won't get merged.
💬 l0rinc commented on pull request "truc: optimize the in package relation calculation":
(https://github.com/bitcoin/bitcoin/pull/33062#discussion_r2252817773)
what's the reason for reusing the set here? This will just create a dependency between the loops. If you have some benchmarks, it would be good to double-check, but this might be simpler (and maybe even faster):
```C++
std::unordered_set<size_t> pindexes;
pindexes.reserve(tx->vin.size()); // TODO or whatever size you expect
```
(https://github.com/bitcoin/bitcoin/pull/33062#discussion_r2252817773)
what's the reason for reusing the set here? This will just create a dependency between the loops. If you have some benchmarks, it would be good to double-check, but this might be simpler (and maybe even faster):
```C++
std::unordered_set<size_t> pindexes;
pindexes.reserve(tx->vin.size()); // TODO or whatever size you expect
```
💬 l0rinc commented on pull request "truc: optimize the in package relation calculation":
(https://github.com/bitcoin/bitcoin/pull/33062#discussion_r2252818078)
we could probably use the newer version here:
```suggestion
std::ranges::copy(pindexes, std::back_inserter(relations[i]));
```
----
I haven't checked the usages, but why are we storing these in a `vector` in the first place if they have a uniqueness constraint?
(https://github.com/bitcoin/bitcoin/pull/33062#discussion_r2252818078)
we could probably use the newer version here:
```suggestion
std::ranges::copy(pindexes, std::back_inserter(relations[i]));
```
----
I haven't checked the usages, but why are we storing these in a `vector` in the first place if they have a uniqueness constraint?
💬 l0rinc commented on pull request "truc: optimize the in package relation calculation":
(https://github.com/bitcoin/bitcoin/pull/33062#discussion_r2252828381)
we don't usually `auto` parameters and not sure why we're copying the result:
```suggestion
inline const std::vector<size_t>& FindInPackageParents(size_t tx_index, const std::vector<std::vector<size_t>>& relations)
{
return relations[tx_index];
}
```
(https://github.com/bitcoin/bitcoin/pull/33062#discussion_r2252828381)
we don't usually `auto` parameters and not sure why we're copying the result:
```suggestion
inline const std::vector<size_t>& FindInPackageParents(size_t tx_index, const std::vector<std::vector<size_t>>& relations)
{
return relations[tx_index];
}
```