💬 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()))`
`
💬 ryanofsky commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1194511676)
In commit "wallet: Add GetPrefixCursor to DatabaseBatch" (42cce1d43be0e662783abf4af60283c352297eac)
I think you can just do:
```c++
m_cursor_end = records.upper_bound(SerializeData(prefix.begin(), prefix.end()));
```
or
```c++
std::tie(m_cursor, m_cursor_end) = records.equal_range(SerializeData(prefix.begin(), prefix.end()));
```
And don't need the for loop.
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1194511676)
In commit "wallet: Add GetPrefixCursor to DatabaseBatch" (42cce1d43be0e662783abf4af60283c352297eac)
I think you can just do:
```c++
m_cursor_end = records.upper_bound(SerializeData(prefix.begin(), prefix.end()));
```
or
```c++
std::tie(m_cursor, m_cursor_end) = records.equal_range(SerializeData(prefix.begin(), prefix.end()));
```
And don't need the for loop.
💬 ryanofsky commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1194495172)
In commit "wallet: Add GetPrefixCursor to DatabaseBatch" (42cce1d43be0e662783abf4af60283c352297eac)
Using const references here forces vectors to be copied. Passing the argument by value would give the caller option to avoid copies:
```c++
explicit SQLiteCursor(std::vector<std::byte> start_range, std::vector<std::byte> end_range)
: m_prefix_range_start(std::move(start_range)),
m_prefix_range_end(std::move(end_range))
{}
```
Passing arguments by value instead
...
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1194495172)
In commit "wallet: Add GetPrefixCursor to DatabaseBatch" (42cce1d43be0e662783abf4af60283c352297eac)
Using const references here forces vectors to be copied. Passing the argument by value would give the caller option to avoid copies:
```c++
explicit SQLiteCursor(std::vector<std::byte> start_range, std::vector<std::byte> end_range)
: m_prefix_range_start(std::move(start_range)),
m_prefix_range_end(std::move(end_range))
{}
```
Passing arguments by value instead
...
💬 Shekelme commented on issue "Stuck in Endless Pre-Syncing Headers Loop":
(https://github.com/bitcoin/bitcoin/issues/26391#issuecomment-1548932081)
Same bug here. v24.0.1
[Endless_Pre-synchronizing.txt](https://github.com/bitcoin/bitcoin/files/11483805/Endless_Pre-synchronizing.txt)
(https://github.com/bitcoin/bitcoin/issues/26391#issuecomment-1548932081)
Same bug here. v24.0.1
[Endless_Pre-synchronizing.txt](https://github.com/bitcoin/bitcoin/files/11483805/Endless_Pre-synchronizing.txt)
💬 vasild commented on pull request "net: support unix domain sockets for -proxy and -onion":
(https://github.com/bitcoin/bitcoin/pull/27375#discussion_r1166806386)
Unrelated change, I am not sure I fully understand. Is it the case that before the message was `Define this symbol to build code that uses getauxval)` and this change just fixes the message to not have a trailing `)`?
(https://github.com/bitcoin/bitcoin/pull/27375#discussion_r1166806386)
Unrelated change, I am not sure I fully understand. Is it the case that before the message was `Define this symbol to build code that uses getauxval)` and this change just fixes the message to not have a trailing `)`?