📝 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
...

💬 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.

📝 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
...

💬 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();
```

💬 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!

💬 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.

💬 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

💬 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.

👍 brunoerg approved a pull request: "walletdb: Remove unused CreateMockWalletDatabase"
(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`.

💬 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
...

💬 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

💬 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()))`
`

💬 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.

💬 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
...

💬 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)

💬 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 `)`?

💬 MarcoFalke commented on issue "Drop support for g++-8?":
(https://github.com/bitcoin/bitcoin/issues/27537#issuecomment-1549169559)
Closing for now. Let's continue discussion in https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109871 and #https://github.com/bitcoin/bitcoin/pull/27662

✅ MarcoFalke closed an issue: "Drop support for g++-8?"
(https://github.com/bitcoin/bitcoin/issues/27537)

📝 MarcoFalke opened a pull request: "ci: Remove unused errtrace trap ERR"
(https://github.com/bitcoin/bitcoin/pull/27667)
This was added in commit 069752b72613b772a9536a3e7f15fa75097f2946, presumably at a time when the functional tests wouldn't capture stderr.

Now that all tests capture and print stderr on failure, it can be removed. Reference:

* Unit tests capture via `2>&1`:

https://github.com/bitcoin/bitcoin/blob/d7700d3a26478d9b1648463c188648c7047b1c60/src/Makefile.test.include#L421

* Functional tests capture as well:

https://github.com/bitcoin/bitcoin/blob/d7700d3a26478d9b1648463c188648c7047b1c6
...