💬 Sjors commented on pull request "Log successful AcceptBlockHeader()":
(https://github.com/bitcoin/bitcoin/pull/27276#issuecomment-1475297700)
At this point I prefer #27278, but I'll leave this open in case that PR doesn't make it. This one is (slightly) easier to review.

👍 Sjors approved a pull request: "Log new headers"
(https://github.com/bitcoin/bitcoin/pull/27278)
Concept ACK

Unconditionally logging headers (outside of IBD) makes sense to me. It doubles the number of messages per block, but since we check their proof-of-work, it's not too bad.

Similarly for compact blocks we check the PoW before logging.

utACK dbd144206a491e3e16d662d0daeffac71bb5b8c7

💬 Sjors commented on pull request "Log new headers":
(https://github.com/bitcoin/bitcoin/pull/27278#discussion_r1141396013)
Maybe add an anti-footgun note here, since I almost made that mistake:

```
// Not all valid headers have a known height, so we don't log pindex->nHeight.
```

💬 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