💬 Sjors commented on pull request "Move log messages: tx enqueue to mempool, allocation to blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27277#discussion_r1141379744)
(added a note to the PR description)

💬 Sjors commented on pull request "Move log messages: tx enqueue to mempool, allocation to blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27277#discussion_r1141380000)
I'll look into switching to the `LogPrintLevel` function.

💬 dergoegge commented on pull request "p2p: Prevent block index fingerprinting by sending additional getheaders messages":
(https://github.com/bitcoin/bitcoin/pull/24571#issuecomment-1475286226)
@stratospher Thank you for having a look at this! There is an alternative approach to the one in this PR that I have been meaning to implement and I forgot to put this PR in draft mode.

We made some changes to the headers sync logic (https://github.com/bitcoin/bitcoin/pull/25454), which causes us to now only have one `getheaders` request in flight for the same peer. If we start storing the locator that is sent in the `getheaders` message and only allow `headers` in response that connect to th
...

📝 dergoegge converted_to_draft a pull request: "p2p: Prevent block index fingerprinting by sending additional getheaders messages"
(https://github.com/bitcoin/bitcoin/pull/24571)
The block index might contain stale blocks that are not part of the main chain. If a malicious peer is able to probe a node's block index for certain stale blocks then it can use this information to fingerprint the node.

When receiving headers (either through a `cmpctblock` or `headers` messages) a node will send `getheaders` if the predecessor of the first header does not exist. This leaks information from the block index if the predecessor of the header is a stale block because no `getheade
...

💬 ryanofsky commented on pull request "rpc, test: remove newline escape sequence from wallet warning fields":
(https://github.com/bitcoin/bitcoin/pull/27138#issuecomment-1475286648)
> I'm not sure it makes sense to patch around this by adding code to our CLI to parse newline escape sequences

Yes agree no need for this as we already have a JSON parser. CLI could see simply if check result object has `"warning"` field which is a string, and if it does, write the string to stderr.

This would be really simple, and make warnings more obvious and readable, and be compatible with other bitcoin-cli improvements in the future.

💬 Sjors commented on pull request "Move log messages: tx enqueue to mempool, allocation to blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27277#discussion_r1141394521)
Done

💬 Sjors commented on pull request "Log successful AcceptBlockHeader()":
(https://github.com/bitcoin/bitcoin/pull/27276#discussion_r1141395250)
Done

💬 jonatack commented on pull request "rpc, test: remove newline escape sequence from wallet warning fields":
(https://github.com/bitcoin/bitcoin/pull/27138#issuecomment-1475293514)
If these four remaining "warning" fields are behind a deprecation flag (opening a PR today), hopefully that avoids the need to do anything further.

💬 Sjors commented on pull request "Log new headers":
(https://github.com/bitcoin/bitcoin/pull/27278#issuecomment-1475297075)
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 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