💬 achow101 commented on pull request "[24.x] qt: 24.1rc3 translations update":
(https://github.com/bitcoin/bitcoin/pull/27627#issuecomment-1544190749)
post merge ACK a86b45cafaabd8eb4957b84a0d70484a597eacc7

Diff is the same.

💬 theuni commented on pull request "build: LLVM 15 & LLD based macOS toolchain":
(https://github.com/bitcoin/bitcoin/pull/21778#issuecomment-1544190999)
@fanquake Do you happen to know what the earliest working clang+lld combo would be? Obviously 15 and 16 work, did 13 or 14 have the required options? Just curious because it'll be good to know what the minimum supported vanilla combo (outside of guix/depends) is as that'll become our new requirement.

💬 Sjors commented on pull request "[23.2] Backports for rc1":
(https://github.com/bitcoin/bitcoin/pull/27624#issuecomment-1544203102)
utACK ce8f812b0ac0905c26edd826c57886a08079b4a7

💬 sr-gi commented on pull request "net processing: avoid serving non-announced txs as a result of a MEMPOOL message":
(https://github.com/bitcoin/bitcoin/pull/27602#issuecomment-1544205452)
> Is it the case that this PR will change the behavior in the following two cases (from the PR title and description I got the impression that it is wider scope):
>
> ### Case 1
> * the node receives `MEMPOOL` request and replies to it
> * in the same second a new transaction enters the mempool (after the `MEMPOOL` message)
> * the node receives `GETDATA` for that transaction
>
> In `master` it would send the transaction even though it never advertised it with `INV` due to the `txinfo.m
...

👍 MarcoFalke approved a pull request: "Fee estimation: avoid serving stale fee estimate"
(https://github.com/bitcoin/bitcoin/pull/27622#pullrequestreview-1422877741)
lgtm. Left some nits

💬 MarcoFalke commented on pull request "Fee estimation: avoid serving stale fee estimate":
(https://github.com/bitcoin/bitcoin/pull/27622#discussion_r1191350003)
nit (feel free to ignore): An alternative to adding a function here would be to call the already existing `Ticks<std::chrono::hours>(a)` over `count_hours(a)`, but either seems fine.

💬 MarcoFalke commented on pull request "Fee estimation: avoid serving stale fee estimate":
(https://github.com/bitcoin/bitcoin/pull/27622#discussion_r1191352778)
nit:

```suggestion
fee_dat = self.nodes[0].chain_path / "fee_estimates.dat"
```

💬 MarcoFalke commented on pull request "Fee estimation: avoid serving stale fee estimate":
(https://github.com/bitcoin/bitcoin/pull/27622#discussion_r1191357213)
nit: I am not a fan of uninterruptible sleep in tests. `assert_debug_log` can be used to wait, if you supply a timeout argument (or rely on the default timeout)

💬 MarcoFalke commented on pull request "Improve performance of p2p inv to send queues":
(https://github.com/bitcoin/bitcoin/pull/27610#discussion_r1191369870)
Thanks, I should have just read the documentation: https://en.cppreference.com/w/cpp/named_req/Compare , which would have answered this.

⚠️ Sataur opened an issue: "guiutil.cpp: formatNiceTimeOffset"
(https://github.com/bitcoin-core/gui/issues/728)
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

last received block was generated _ years and 0 weeks..

### Expected behaviour

that tool tip can be improved by:
_..you first see the [raw](https://4sysops.com/archives/format-time-and-date-output-of-powershell-new-timespan/) output and then the human-friendly version created by my.._
_Only non-zero weeks, days, hours, minutes and seconds are displayed_

### Steps to reproduce

_Submit
...

🤔 ajtowns reviewed a pull request: "Parallel compact block downloads, take 3"
(https://github.com/bitcoin/bitcoin/pull/27626#pullrequestreview-1422727983)
Seems okay at first glance. It would be nice to have a "blockrequest.cpp" module that collects all this logic together. Perhaps we want to keep this change simple so it's a smaller backport, and refactor later, though.

💬 ajtowns commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1191251631)
Would this be better as a `map<uint256, map<NodeId, list<>::iterator>>` ? Seems like it'd make some of the `FIXME`s easier.

💬 ajtowns commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1191307770)
I think this case is (mostly) for when a non-high-bandwidth peer tells us about a block, we request it via a compact block, and then we get a different block from some other peer, UpdateTip to that, and only afterwards see the CMPCTBLOCK response from the original peer. But now that response is probably useless, so replace it with a GETDATA.

I wonder if that might be better expressed as: `if (in_flight_same_peer && pindex->pprev != ActiveChain().Tip())` ?

💬 ajtowns commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1191313364)
Might be better to name this `requested_block_from_this_peer` ? (I think for new code we prefer `bool x{false};` so we get errors if we're accidently narrowing values?)

💬 ajtowns commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1191357478)
Out of the last 612 compact blocks my node's reconstructed that required requesting transactions, 277 needed 10 or fewer (45%), 441 needed 50 or fewer (72%), 488 needed 100 or fewer (80%), 548 needed 500 or fewer (90%). I'd probably prefer 50 or 100 than 10, I guess?

(In simpler times, 1086/1162 (93%) need 10 or fewer, 1140/1162 (98%) needed 50 or fewer, 1144/1162 (98.5%) needed 100 or fewer. Again, I'm not including blocks that requested 0 txs in those denominators)

💬 ajtowns commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1191323390)
(Test the bool first?)

💬 ajtowns commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1191320979)
`already_in_flight == 0 || (already_in_flight == 1 && in_flight_same_peer)` ?

💬 ajtowns commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1191369251)
This logic is different to the previous `first_in_flight`; probably should be the same?

💬 instagibbs commented on pull request "rpc: allow submitpackage to be called outside of regtest":
(https://github.com/bitcoin/bitcoin/pull/27609#discussion_r1191393064)
Lots of magic numbers in this test. Could you assert specific vsizes on each tx, and have that number match the picture here?

💬 instagibbs commented on pull request "rpc: allow submitpackage to be called outside of regtest":
(https://github.com/bitcoin/bitcoin/pull/27609#discussion_r1191360507)
test/txpackage_tests.cpp:382:9: warning: multi-line comment [-Wcomment]
382 | // / \
| ^