🤔 glozow reviewed a pull request: "mempool: Persist with XOR"
(https://github.com/bitcoin/bitcoin/pull/28207#pullrequestreview-1722565783)
ACK fa520848da
Code LGTM, tried a few roundtrips of stop/start/savemempool/importmempool with combinations of the options. I originally thought `-persistmempool=0` should imply something for `-persistmempoolv1` but makes sense that it applies to `savemempool` when we're not persisting.
(https://github.com/bitcoin/bitcoin/pull/28207#pullrequestreview-1722565783)
ACK fa520848da
Code LGTM, tried a few roundtrips of stop/start/savemempool/importmempool with combinations of the options. I originally thought `-persistmempool=0` should imply something for `-persistmempoolv1` but makes sense that it applies to `savemempool` when we're not persisting.
💬 glozow commented on pull request "mempool: Persist with XOR":
(https://github.com/bitcoin/bitcoin/pull/28207#discussion_r1388024702)
nit: would be nice to name 1 as `MEMPOOL_DUMP_VERSION_V1` or `MEMPOOL_DUMP_VERSION_NO_XOR`
(https://github.com/bitcoin/bitcoin/pull/28207#discussion_r1388024702)
nit: would be nice to name 1 as `MEMPOOL_DUMP_VERSION_V1` or `MEMPOOL_DUMP_VERSION_NO_XOR`
💬 glozow commented on pull request "mempool: Persist with XOR":
(https://github.com/bitcoin/bitcoin/pull/28207#discussion_r1388019621)
Should the mempool_compatibility.py test be updated? (the comment "200100 # Last release with previous mempool format" isn't accurate anymore)
(https://github.com/bitcoin/bitcoin/pull/28207#discussion_r1388019621)
Should the mempool_compatibility.py test be updated? (the comment "200100 # Last release with previous mempool format" isn't accurate anymore)
💬 instagibbs commented on pull request "fuzz: Minor improvements to tx_package_eval target":
(https://github.com/bitcoin/bitcoin/pull/28825#discussion_r1388055793)
done
(https://github.com/bitcoin/bitcoin/pull/28825#discussion_r1388055793)
done
💬 furszy commented on issue "TSAN: lock-order-inversion (potential deadlock) in ZapSelectTx test":
(https://github.com/bitcoin/bitcoin/issues/27582#issuecomment-1803902266)
Saw this too late, but just in case this appears again; I have rewritten `ZapSelectTx()` entirely inside #28574.
(https://github.com/bitcoin/bitcoin/issues/27582#issuecomment-1803902266)
Saw this too late, but just in case this appears again; I have rewritten `ZapSelectTx()` entirely inside #28574.
📝 darosior opened a pull request: "fuzz: rule-out too deep derivation paths in descriptor parsing targets"
(https://github.com/bitcoin/bitcoin/pull/28832)
This fixes the `mocked_descriptor_parse` timeout reported in #28812 and direct the targets more toward what they are intended to fuzz: the descriptor syntax.
(https://github.com/bitcoin/bitcoin/pull/28832)
This fixes the `mocked_descriptor_parse` timeout reported in #28812 and direct the targets more toward what they are intended to fuzz: the descriptor syntax.
🤔 stickies-v reviewed a pull request: "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access"
(https://github.com/bitcoin/bitcoin/pull/28391#pullrequestreview-1624659971)
Code Review ACK 2be5e799ba623f969f5ffc59667a5bc6799df290
Left a number of suggestions: some about performance improvements, some about avoiding UB, and some general style/concise nits, but they can all be done in a follow-up too.
(https://github.com/bitcoin/bitcoin/pull/28391#pullrequestreview-1624659971)
Code Review ACK 2be5e799ba623f969f5ffc59667a5bc6799df290
Left a number of suggestions: some about performance improvements, some about avoiding UB, and some general style/concise nits, but they can all be done in a follow-up too.
💬 stickies-v commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1380338654)
`infoAll()` holds its own `CTxMempool::cs` lock, and I think we don't need that to add stuff to `notifications`? I'm also not quite sure if we need to be holding `::cs_main` here?
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1380338654)
`infoAll()` holds its own `CTxMempool::cs` lock, and I think we don't need that to add stuff to `notifications`? I'm also not quite sure if we need to be holding `::cs_main` here?
💬 stickies-v commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1387913882)
nit: I think a brief rationale behind why we need this change would be good to add to the commit message of "[refactor] rewrite vTxHashes as a vector of CTransactionRef"
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1387913882)
nit: I think a brief rationale behind why we need this change would be good to add to the commit message of "[refactor] rewrite vTxHashes as a vector of CTransactionRef"
💬 stickies-v commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1387907915)
nit: one-liners seem appropriate? (+ [here](https://github.com/bitcoin/bitcoin/pull/28391/files#diff-97c3a52bc5fad452d82670a7fd291800bae20c7bc35bb82686c2c0a4ea7b5b98R1487), [here](https://github.com/bitcoin/bitcoin/pull/28391/files#diff-97c3a52bc5fad452d82670a7fd291800bae20c7bc35bb82686c2c0a4ea7b5b98R1498))
```suggestion
const auto entry{Assert(pool.GetEntry(tx.GetHash()))};
```
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1387907915)
nit: one-liners seem appropriate? (+ [here](https://github.com/bitcoin/bitcoin/pull/28391/files#diff-97c3a52bc5fad452d82670a7fd291800bae20c7bc35bb82686c2c0a4ea7b5b98R1487), [here](https://github.com/bitcoin/bitcoin/pull/28391/files#diff-97c3a52bc5fad452d82670a7fd291800bae20c7bc35bb82686c2c0a4ea7b5b98R1498))
```suggestion
const auto entry{Assert(pool.GetEntry(tx.GetHash()))};
```
💬 stickies-v commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1387904513)
nit: could simplify
```suggestion
const auto entry{CHECK_NONFATAL(pool.GetEntry(hash))};
```
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1387904513)
nit: could simplify
```suggestion
const auto entry{CHECK_NONFATAL(pool.GetEntry(hash))};
```
💬 stickies-v commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1387953875)
It's probably not too important, but this is a bit of a performance and memory regression to construct the vector of `TxMempoolInfo` when really we only need the underlying txref.
I think it could make sense to add `std::vector<const CTxMemPoolEntry*> entryAll() const EXCLUSIVE_LOCKS_REQUIRED(cs);` to reduce memory and computational overhead and as a bonus make the code a bit easier to read when we just want to iterate over the mempool entries?
<details>
<summary>git diff on 2be5e799ba</s
...
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1387953875)
It's probably not too important, but this is a bit of a performance and memory regression to construct the vector of `TxMempoolInfo` when really we only need the underlying txref.
I think it could make sense to add `std::vector<const CTxMemPoolEntry*> entryAll() const EXCLUSIVE_LOCKS_REQUIRED(cs);` to reduce memory and computational overhead and as a bonus make the code a bit easier to read when we just want to iterate over the mempool entries?
<details>
<summary>git diff on 2be5e799ba</s
...
💬 stickies-v commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1387920369)
nit: Given that we're doing not any meaninful operations with `i` I think using `auto` makes sense, and the ternary operator also seems appropriate here, simplifying this to:
```suggestion
const auto i{mapTx.find(txid)};
return i == mapTx.end() ? nullptr : &(*i);
```
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1387920369)
nit: Given that we're doing not any meaninful operations with `i` I think using `auto` makes sense, and the ternary operator also seems appropriate here, simplifying this to:
```suggestion
const auto i{mapTx.find(txid)};
return i == mapTx.end() ? nullptr : &(*i);
```
💬 stickies-v commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1387927635)
This seems like a slight regression: `tx3_iter.value()` would throw if the hash was not found, whereas `tx3_entry->GetSizeWithAncestors()` is UB. I think it would be good to add inline `Assert`s to the entry initialization (and add list initialization while you're at it)?
```cpp
const auto tx3_entry{Assert(pool.GetEntry(tx3->GetHash()))};
```
Similar concerns [here](https://github.com/bitcoin/bitcoin/pull/28391/files#diff-e9d32ba5cee0e31252a337dda9c2272152be7e9798601e62c2d042dccf79f7c1R9
...
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1387927635)
This seems like a slight regression: `tx3_iter.value()` would throw if the hash was not found, whereas `tx3_entry->GetSizeWithAncestors()` is UB. I think it would be good to add inline `Assert`s to the entry initialization (and add list initialization while you're at it)?
```cpp
const auto tx3_entry{Assert(pool.GetEntry(tx3->GetHash()))};
```
Similar concerns [here](https://github.com/bitcoin/bitcoin/pull/28391/files#diff-e9d32ba5cee0e31252a337dda9c2272152be7e9798601e62c2d042dccf79f7c1R9
...
💬 stickies-v commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1388021604)
nit: not a regression, but using `.value()` seems safer (also for `tx7`, `tx8`, `tx9` further down)
```suggestion
setAncestors.insert(pool.GetIter(tx6.GetHash()).value());
```
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1388021604)
nit: not a regression, but using `.value()` seems safer (also for `tx7`, `tx8`, `tx9` further down)
```suggestion
setAncestors.insert(pool.GetIter(tx6.GetHash()).value());
```
💬 stickies-v commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1388039482)
nit: I think we can just directly use `GetIter`?
```suggestion
GetIter(txns_randomized.back()->GetHash()).value()->idx_randomized = it->idx_randomized;
```
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1388039482)
nit: I think we can just directly use `GetIter`?
```suggestion
GetIter(txns_randomized.back()->GetHash()).value()->idx_randomized = it->idx_randomized;
```
📝 theStack opened a pull request: "wallet: refactor: remove unused `SignatureData` instances in spkm's `FillPSBT` methods"
(https://github.com/bitcoin/bitcoin/pull/28833)
These are filled with signature data from a PSBT input, but not used anywhere after, hence they can be removed. Note that the same code is in the `SignPSBTInput` function where the `sigdata` result is indeed used.
(https://github.com/bitcoin/bitcoin/pull/28833)
These are filled with signature data from a PSBT input, but not used anywhere after, hence they can be removed. Note that the same code is in the `SignPSBTInput` function where the `sigdata` result is indeed used.
💬 furszy commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1388101787)
In 5809c90fee6:
Need to remove this flag. Otherwise the migrated watch-only and solvables wallets will have the global hd key flag set while newly created private key disabled wallets will not.
This is because the wallets are created calling `CWallet::Create()` here, which takes this variable and set it calling `CWallet ::InitWalletFlags()`.
It would be nice to add a test verifying that we do not create a private key disabled wallet with the global hd key flag set.
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1388101787)
In 5809c90fee6:
Need to remove this flag. Otherwise the migrated watch-only and solvables wallets will have the global hd key flag set while newly created private key disabled wallets will not.
This is because the wallets are created calling `CWallet::Create()` here, which takes this variable and set it calling `CWallet ::InitWalletFlags()`.
It would be nice to add a test verifying that we do not create a private key disabled wallet with the global hd key flag set.
💬 brunoerg commented on pull request "fuzz: call lookup functions before calling `Ban`":
(https://github.com/bitcoin/bitcoin/pull/27935#issuecomment-1804028996)
> Ah ok. And about the efficiency, is it possible to keep the fuzz input format unchanged instead of using a string representation (Maybe via a string-lookup roundtrip)? I wonder if that'd be more efficient.
Yes, force-pushed addressing it. Also, I think this way our seeds remain great.
(https://github.com/bitcoin/bitcoin/pull/27935#issuecomment-1804028996)
> Ah ok. And about the efficiency, is it possible to keep the fuzz input format unchanged instead of using a string representation (Maybe via a string-lookup roundtrip)? I wonder if that'd be more efficient.
Yes, force-pushed addressing it. Also, I think this way our seeds remain great.
💬 Riahiamirreza commented on pull request "rpc: show P2(W)SH redeemScript in getrawtransaction #27637":
(https://github.com/bitcoin/bitcoin/pull/27638#issuecomment-1804074931)
@Sjors For adding new field to a transaction (in my case, adding `redeemScript` to `vin`) is it correct to add new fields to `CTxIn` class in `test/functional/test_framework/messages.py`? While adding it will cause mismatch in weight calculation of transactions, I think it's not the correct way. But I'm not sure what's the correct way.
(https://github.com/bitcoin/bitcoin/pull/27638#issuecomment-1804074931)
@Sjors For adding new field to a transaction (in my case, adding `redeemScript` to `vin`) is it correct to add new fields to `CTxIn` class in `test/functional/test_framework/messages.py`? While adding it will cause mismatch in weight calculation of transactions, I think it's not the correct way. But I'm not sure what's the correct way.