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

💬 Sjors commented on issue "Allow getblockfrompeer to use any peer":
(https://github.com/bitcoin/bitcoin/issues/27652#issuecomment-1549226748)
I would like this as well but I was too lazy to implement it.

The downside is that it would make the call asynchronous and potentially very long (needing `-rpcclienttimeout=0`). But this would only be the case if the user _doesn't_ specify a peer.

It would also need a progress indicator, which our RPC doesn't really offer. The debug log can of course be used for that. Or you could go all fancy and have the node track the status of the block fetch, including the ability to cancel it.

The
...

💬 MarcoFalke commented on pull request "Raise on invalid -debug and -loglevel config options":
(https://github.com/bitcoin/bitcoin/pull/27632#discussion_r1194846344)
nit: Any reason to use r-strings when there is no need to and they don't contain any `\` chars?

🚀 fanquake merged a pull request: "walletdb: Remove unused CreateMockWalletDatabase"
(https://github.com/bitcoin/bitcoin/pull/27665)

💬 MarcoFalke commented on pull request "Raise on invalid -debug and -loglevel config options":
(https://github.com/bitcoin/bitcoin/pull/27632#discussion_r1194857223)
q: Can you explain why it is fine to drop the translation?

💬 MarcoFalke commented on pull request "Raise on invalid -debug and -loglevel config options":
(https://github.com/bitcoin/bitcoin/pull/27632#discussion_r1194858242)
Not sure if we want to use `Result` before it supports `<void>`, but I guess it will be less code to change in the future, compared to `std::optional<bilingual_str>`, so I guess it is fine for now.