💬 jonatack commented on pull request "Log new headers":
(https://github.com/bitcoin/bitcoin/pull/27278#discussion_r1141399833)
Can use `LogPrintfCategory` here, if you have a category in mind that this fits into. (Thanks for updating the second logging call.)

💬 Sjors commented on pull request "Log new headers":
(https://github.com/bitcoin/bitcoin/pull/27278#discussion_r1141402261)
Also, let's capitalize the log message: `Saw new …`

💬 martinus commented on pull request "refactor: Use move semantics instead of custom swap functions":
(https://github.com/bitcoin/bitcoin/pull/26749#discussion_r1141442527)
I'd do it this way:

```cpp
queue.insert(queue.end(), std::make_move_iterator(vChecks.begin()), std::make_move_iterator(vChecks.end()));
```

This has the advantage that queue's insert knows the number of elements that are going to be inserted and will reserve enough space, so it's faster

💬 martinus commented on pull request "refactor: Use move semantics instead of custom swap functions":
(https://github.com/bitcoin/bitcoin/pull/26749#discussion_r1141443409)
Instead of the loop I'd use this bulk method:
```cpp
auto start_it = queue.end() - nNow;
vChecks.assign(std::make_move_iterator(start_it), std::make_move_iterator(queue.end()));
queue.erase(start_it, queue.end());
```

Again that should be faster than lots of calls to `push_back` which need to check the capacity, also I find it more readable. Note that the difference is that the queue items will be added in a non-reversed order, but I'd say that should be irrelevant.

I gave that a try,
...

👍 martinus approved a pull request: "refactor: Use move semantics instead of custom swap functions"
(https://github.com/bitcoin/bitcoin/pull/26749)
Code review ACK ff6c0abeea, ran the unit tests, commented performance nits with suggested changes

✅ antonilol closed an issue: "`getblocktemplate` returns a standard P2PKH with 0 sigops (testnet)"
(https://github.com/bitcoin/bitcoin/issues/24255)

💬 amitiuttarwar commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#issuecomment-1475388292)
thank you everyone for the thoughtful feedback! some questions & responses -

### RE: @vasild logic to initially overshoot number of connections

> If we overshoot the 8 connections (like described above), then for some time we will have more, until eviction kicks in. How much time is that exactly? If that is short then it would be sub-optimal to open a IPv4 connection and drop it shortly after in cases where Tor connectivity is working smoothly. If it is long then we have practically increa
...

📝 jonatack opened a pull request: "Add "warnings"/deprecate warning {create,load,unload,restore}wallet, deprecate "warning""
(https://github.com/bitcoin/bitcoin/pull/27279)
null

💬 LarryRuane commented on pull request "util: improve FindByte() performance":
(https://github.com/bitcoin/bitcoin/pull/19690#issuecomment-1475594768)
Force-pushed for review comments (thanks, @stickies-v), verified benchmark performance is unchanged (ns/op with PR: 34.76, without PR: 302). Summary of force-push changes:
- change `FindByte()` argument type from `uint8_t` to `std::byte`)
- add "exception" comment before call to `Fill()`
- add comment suggesting to avoid mod (%) operator within loop
- changed assignment statements to use more modern braces syntax

💬 ajtowns commented on pull request "Move log messages: tx enqueue to mempool, allocation to blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27277#discussion_r1141644603)
> Looking at the history of the `BLOCKSTORE` category, it was introduced as part of a larger change in #23235 to reduce the default log noise. There was no discussion about this particular line, and my guess is that it was overlooked. cc @ajtowns

That sounds right

💬 benma commented on issue "descriptors: represent multiple derivation paths within one descriptor":
(https://github.com/bitcoin/bitcoin/issues/17190#issuecomment-1475704256)
Here is another use case to consider:

The BitBoxApp merges `wpkh(xpub1/<0;1>/*)` and `tr(xpub2/<0;1>/*)` into a single user-facing account, so users don't have to worry about low level things like choosing which script type to use.

While The `<0;1>` syntax is great to merge the receive and change chains into a single output descriptor, it would be great if one could also represent keys derived from different xpubs and script types in a single output descriptor. BIP88 doesn't seem to cover
...

💬 S3RK commented on pull request "wallet: return error msg for "too-long-mempool-chain"":
(https://github.com/bitcoin/bitcoin/pull/24845#discussion_r1141752785)
1) I think this should respect `fRejectLongChains` flag. If user explicitly asked us to ignore long-chain constraints, then we should respect that.

2) If `fRejectLongChains` flag is set (the default) then the most lenient filter has `max_ancestors/2` and `max_descendants/2`. So this condition could never be hit.

In summary I don't yet understand how one can hit a message described in #23144

💬 S3RK commented on pull request "wallet, tests: Expand and test when the blank wallet flag should be un/set":
(https://github.com/bitcoin/bitcoin/pull/25634#discussion_r1141776601)
I have a different perspective. The flag was provided during wallet creation and captures the intent of the user.

> The blank flag is used only to indicate to that code that the wallet is intentionally blank and should not be setup as a new wallet.

I'd like to take this one step further and argue that "The blank flag is used only to indicate to that code that the wallet is intentionally blank and **the user is responsible for managing the keys in this wallet manually**." If user created wa
...

💬 S3RK commented on pull request "wallet, tests: Expand and test when the blank wallet flag should be un/set":
(https://github.com/bitcoin/bitcoin/pull/25634#issuecomment-1475818349)
Concept ACK. But approach wise I'd like us to preserve the intent of the user that the wallet is intended as `manual` (we should a better term). Would it be hard to create a new flag to persist that intent and respect that (e.g on encryption)?

💬 MarcoFalke commented on pull request "Move log messages: tx enqueue to mempool, allocation to blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27277#issuecomment-1475912615)
CI network error https://cirrus-ci.com/task/4864754720702464?logs=ci#L61:

`TimeoutError: [Errno 110] Connection timed out`

💬 MarcoFalke commented on pull request "refactor: Use move semantics instead of custom swap functions":
(https://github.com/bitcoin/bitcoin/pull/26749#issuecomment-1475931039)
re-ACK ff6c0abeea00deffcaeaf3b02092110d6895b1c0 🦀

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: re-ACK ff6c0abeea00deffcaea
...

💬 MarcoFalke commented on pull request "refactor: Use move semantics instead of custom swap functions":
(https://github.com/bitcoin/bitcoin/pull/26749#discussion_r1141877181)
> Commits have been reorganized.

No they haven't?

💬 MarcoFalke commented on pull request "refactor: Use move semantics instead of custom swap functions":
(https://github.com/bitcoin/bitcoin/pull/26749#discussion_r1141878435)
The commit that fixes the use-after-free should come with a commit message to say that it fixed the bug

💬 MarcoFalke commented on pull request "refactor: Use move semantics instead of custom swap functions":
(https://github.com/bitcoin/bitcoin/pull/26749#issuecomment-1475934980)
nit in https://github.com/bitcoin/bitcoin/pull/26749#discussion_r1141878435 is still unfixed?

💬 MarcoFalke commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1141893047)
> cs_main

Hmm, that sounds like you are making it harder to remove cs_main.

I am not sure it the alternative are better, but my brain enjoys the thought of having only to think about a single blockman object at a time. Ideas:

* Use `findBlock` from the node interface. The overhead should only be another map lookup by the block hash? You can ask Ryan to confirm.
* Pass down a blockman ref into the zmq stuff (massive change)
* Add a new global alias for zmq to point to the single existi
...