💬 pinheadmz commented on pull request "test: add unit test coverage for Python ECDSA implementation":
(https://github.com/bitcoin/bitcoin/pull/27653#discussion_r1194287587)
IIUC we don't assert that the keys we expect to be invalid are marked correctly. Seems like we mine as well ensure that `generate_privkey()` is always valid but certain edge cases are not (`0`, `SECP256K1_ORDER`, and `2**256 - 1`)
(https://github.com/bitcoin/bitcoin/pull/27653#discussion_r1194287587)
IIUC we don't assert that the keys we expect to be invalid are marked correctly. Seems like we mine as well ensure that `generate_privkey()` is always valid but certain edge cases are not (`0`, `SECP256K1_ORDER`, and `2**256 - 1`)
🤔 pinheadmz reviewed a pull request: "test: add unit test coverage for Python ECDSA implementation"
(https://github.com/bitcoin/bitcoin/pull/27653#pullrequestreview-1427256339)
concept ACK
I also think it would be cool to see a set of ECDSA test vectors here. We already have a set of BIP340 vectors tested both in python and in the `key_tests.cpp` unit test, assuring that both implementations handle Schnorr (at least those cases) identically.
(https://github.com/bitcoin/bitcoin/pull/27653#pullrequestreview-1427256339)
concept ACK
I also think it would be cool to see a set of ECDSA test vectors here. We already have a set of BIP340 vectors tested both in python and in the `key_tests.cpp` unit test, assuring that both implementations handle Schnorr (at least those cases) identically.
💬 sdaftuar commented on pull request "p2p: Stop relaying non-mempool txs":
(https://github.com/bitcoin/bitcoin/pull/27625#issuecomment-1548505174)
> My understanding is that `REPLACED` would also need to be provided to peers: [#27625 (comment)](https://github.com/bitcoin/bitcoin/pull/27625#issuecomment-1544171255). Otherwise there may be another round trip if the miner mines a previous version of a recently replaced transaction, which is in `mapRelay` and `vExtraTxnForCompact`, but not the mempool (and not yet in a block, because it may be in-flight)?
I think it's better that we not relay transactions that are `REPLACED` -- note that we
...
(https://github.com/bitcoin/bitcoin/pull/27625#issuecomment-1548505174)
> My understanding is that `REPLACED` would also need to be provided to peers: [#27625 (comment)](https://github.com/bitcoin/bitcoin/pull/27625#issuecomment-1544171255). Otherwise there may be another round trip if the miner mines a previous version of a recently replaced transaction, which is in `mapRelay` and `vExtraTxnForCompact`, but not the mempool (and not yet in a block, because it may be in-flight)?
I think it's better that we not relay transactions that are `REPLACED` -- note that we
...
💬 mzumsande commented on pull request "p2p, rpc: Manual block-relay-only connections with addnode":
(https://github.com/bitcoin/bitcoin/pull/24170#discussion_r1194318233)
`m_node_address` is a string, which is part of AddedNodeParams, which has just the unvalidated user input.
`resolvedAddress` is a `CService` built from `m_node_address`, which is generated in `GetAddedNodeInfo` and used in `ThreadOpenConnection` / rpc code.
(https://github.com/bitcoin/bitcoin/pull/24170#discussion_r1194318233)
`m_node_address` is a string, which is part of AddedNodeParams, which has just the unvalidated user input.
`resolvedAddress` is a `CService` built from `m_node_address`, which is generated in `GetAddedNodeInfo` and used in `ThreadOpenConnection` / rpc code.
💬 joostjager commented on pull request "rpc: allow submitpackage to be called outside of regtest":
(https://github.com/bitcoin/bitcoin/pull/27609#issuecomment-1548517395)
There is a bugfix in here, because I think it fixes the submitpackage acceptance policy on regtest by restricting it to a safe subset of topologies? Users may use regtest now to prepare for usage of the rpc on mainnet later.
(https://github.com/bitcoin/bitcoin/pull/27609#issuecomment-1548517395)
There is a bugfix in here, because I think it fixes the submitpackage acceptance policy on regtest by restricting it to a safe subset of topologies? Users may use regtest now to prepare for usage of the rpc on mainnet later.
📝 achow101 opened a pull request: "walletdb: Remove unused CreateMockWalletDatabase"
(https://github.com/bitcoin/bitcoin/pull/27665)
This has been superseded by the MockableDatabase. Remove to avoid confusion as to which type of mock database to use for testing.
I thought this was included in #26715, maybe it got lost in a rebase.
(https://github.com/bitcoin/bitcoin/pull/27665)
This has been superseded by the MockableDatabase. Remove to avoid confusion as to which type of mock database to use for testing.
I thought this was included in #26715, maybe it got lost in a rebase.
👍 furszy approved a pull request: "walletdb: Remove unused CreateMockWalletDatabase"
(https://github.com/bitcoin/bitcoin/pull/27665#pullrequestreview-1427332880)
utACK 0282b212
(https://github.com/bitcoin/bitcoin/pull/27665#pullrequestreview-1427332880)
utACK 0282b212
📝 achow101 opened a pull request: "wallet, bench: Move commonly used functions to their own file and fix a bug"
(https://github.com/bitcoin/bitcoin/pull/27666)
I have a few PRs and branches that use these two commits, probably makes sense to split them into a separate PR to be merged sooner.
The first commit contains some things that end up being commonly used in new wallet benchmarks. These are moved into `wallet_common.{h/cpp}`.
The second commit contains a bugfix for the wallet_balance benchmark where it calls `LoadWallet` in the wrong place. It's unnecessary to call that function in this benchmark. Although this does not cause any issues curr
...
(https://github.com/bitcoin/bitcoin/pull/27666)
I have a few PRs and branches that use these two commits, probably makes sense to split them into a separate PR to be merged sooner.
The first commit contains some things that end up being commonly used in new wallet benchmarks. These are moved into `wallet_common.{h/cpp}`.
The second commit contains a bugfix for the wallet_balance benchmark where it calls `LoadWallet` in the wrong place. It's unnecessary to call that function in this benchmark. Although this does not cause any issues curr
...
💬 achow101 commented on pull request "wallet: cache IsMine scriptPubKeys to improve performance of descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/26008#issuecomment-1548545937)
First 2 commits split into #27666. Marking as draft for now.
(https://github.com/bitcoin/bitcoin/pull/26008#issuecomment-1548545937)
First 2 commits split into #27666. Marking as draft for now.
📝 achow101 converted_to_draft a pull request: "wallet: cache IsMine scriptPubKeys to improve performance of descriptor wallets"
(https://github.com/bitcoin/bitcoin/pull/26008)
Wallets that have a ton of non-ranged descriptors (such as a migrated non-HD wallet) perform fairly poorly due to looping through all of the wallet's `ScriptPubKeyMan`s. This is done in various places, such as `IsMine`, and helper functions for fetching a `ScriptPubKeyMan` and a `SolvingProvider`. This also has a bit of a performance impact on standard descriptor wallets, although less noticeable due to the small number of SPKMs.
As these functions are based on doing `IsMine` for each `Script
...
(https://github.com/bitcoin/bitcoin/pull/26008)
Wallets that have a ton of non-ranged descriptors (such as a migrated non-HD wallet) perform fairly poorly due to looping through all of the wallet's `ScriptPubKeyMan`s. This is done in various places, such as `IsMine`, and helper functions for fetching a `ScriptPubKeyMan` and a `SolvingProvider`. This also has a bit of a performance impact on standard descriptor wallets, although less noticeable due to the small number of SPKMs.
As these functions are based on doing `IsMine` for each `Script
...
💬 ismaelsadeeq commented on pull request "RPC: Accept options as named-only parameters":
(https://github.com/bitcoin/bitcoin/pull/26485#discussion_r1194346683)
Concept ACK
Nit: As mentioned in review club, it would be nice to use a more descriptive variable name for the fr variable, considering your change is within the `transformNamedArguments` method. like find_result or a similar alternative.
```suggestion
auto find_result = argsIn.end();
```
(https://github.com/bitcoin/bitcoin/pull/26485#discussion_r1194346683)
Concept ACK
Nit: As mentioned in review club, it would be nice to use a more descriptive variable name for the fr variable, considering your change is within the `transformNamedArguments` method. like find_result or a similar alternative.
```suggestion
auto find_result = argsIn.end();
```
💬 mzumsande commented on pull request "p2p, rpc: Manual block-relay-only connections with addnode":
(https://github.com/bitcoin/bitcoin/pull/24170#discussion_r1194346880)
Yes, thanks, that's a bit simpler and should be equivalent, so I took your suggestion!
(https://github.com/bitcoin/bitcoin/pull/24170#discussion_r1194346880)
Yes, thanks, that's a bit simpler and should be equivalent, so I took your suggestion!
💬 mzumsande commented on pull request "p2p, rpc: Manual block-relay-only connections with addnode":
(https://github.com/bitcoin/bitcoin/pull/24170#discussion_r1194347292)
made obsolete by the next comment.
(https://github.com/bitcoin/bitcoin/pull/24170#discussion_r1194347292)
made obsolete by the next comment.
💬 mzumsande commented on pull request "p2p, rpc: Manual block-relay-only connections with addnode":
(https://github.com/bitcoin/bitcoin/pull/24170#issuecomment-1548552800)
[c13c986 ](https://github.com/bitcoin/bitcoin/commit/c13c9864c8f50a74dfca8feba42e2d28831decaf)to [41ce312](https://github.com/bitcoin/bitcoin/commit/41ce31289fdb521d3f3af1b67f750ce4a9196391):
Incorporated feedback by @brunoerg
(https://github.com/bitcoin/bitcoin/pull/24170#issuecomment-1548552800)
[c13c986 ](https://github.com/bitcoin/bitcoin/commit/c13c9864c8f50a74dfca8feba42e2d28831decaf)to [41ce312](https://github.com/bitcoin/bitcoin/commit/41ce31289fdb521d3f3af1b67f750ce4a9196391):
Incorporated feedback by @brunoerg
💬 brunoerg commented on pull request "walletdb: Remove unused CreateMockWalletDatabase":
(https://github.com/bitcoin/bitcoin/pull/27665#issuecomment-1548585079)
I've used `CreateMockWalletDatabase` in #27647, didn't know it hasn't been used in any other place, going to change it there.
(https://github.com/bitcoin/bitcoin/pull/27665#issuecomment-1548585079)
I've used `CreateMockWalletDatabase` in #27647, didn't know it hasn't been used in any other place, going to change it there.
👍 brunoerg approved a pull request: "walletdb: Remove unused CreateMockWalletDatabase"
(https://github.com/bitcoin/bitcoin/pull/27665#pullrequestreview-1427374841)
crACK 0282b2126dcc1216a25417db0716a3a28489b72d
(https://github.com/bitcoin/bitcoin/pull/27665#pullrequestreview-1427374841)
crACK 0282b2126dcc1216a25417db0716a3a28489b72d
💬 brunoerg commented on pull request "fuzz: wallet, add target for `fees`":
(https://github.com/bitcoin/bitcoin/pull/27647#issuecomment-1548707391)
Force-pushed to replace `CreateMockWalletDatabase`for `CreateMockableWalletDatabase`.
(https://github.com/bitcoin/bitcoin/pull/27647#issuecomment-1548707391)
Force-pushed to replace `CreateMockWalletDatabase`for `CreateMockableWalletDatabase`.
💬 ariard commented on pull request "p2p: Increase tx relay rate":
(https://github.com/bitcoin/bitcoin/pull/27630#issuecomment-1548841340)
> Using 11 tx/second as an estimate for the max network throughput, this commit bumps the rate up to 22 tx/s (for inbound peers), providing a factor of 2 safety margin.
The 11tx/second proposed is the max network throughput from a full-node with default and random transaction-relay topology to the miners or something else ? And what is the size of small transactions which make it sustainable for higher relay rate ? I think we can adjust rebroadcast frequency in function of those parameters on
...
(https://github.com/bitcoin/bitcoin/pull/27630#issuecomment-1548841340)
> Using 11 tx/second as an estimate for the max network throughput, this commit bumps the rate up to 22 tx/s (for inbound peers), providing a factor of 2 safety margin.
The 11tx/second proposed is the max network throughput from a full-node with default and random transaction-relay topology to the miners or something else ? And what is the size of small transactions which make it sustainable for higher relay rate ? I think we can adjust rebroadcast frequency in function of those parameters on
...
💬 ryanofsky commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1194474475)
In commit "wallet: Add GetPrefixCursor to DatabaseBatch" (42cce1d43be0e662783abf4af60283c352297eac)
Would be good to log an error if this fails
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1194474475)
In commit "wallet: Add GetPrefixCursor to DatabaseBatch" (42cce1d43be0e662783abf4af60283c352297eac)
Would be good to log an error if this fails
💬 ryanofsky commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1194486646)
In commit "wallet: Add GetPrefixCursor to DatabaseBatch" (42cce1d43be0e662783abf4af60283c352297eac)
C++ doesn't actually require that a byte is 8 bits, so it would be more correct to write this as `if (*it == std::byte(std::numeric_limits<unsigned char>::max()))`
`
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1194486646)
In commit "wallet: Add GetPrefixCursor to DatabaseBatch" (42cce1d43be0e662783abf4af60283c352297eac)
C++ doesn't actually require that a byte is 8 bits, so it would be more correct to write this as `if (*it == std::byte(std::numeric_limits<unsigned char>::max()))`
`