💬 mzumsande commented on issue "EstimateMedianVal returns higher fee for higher confTarget":
(https://github.com/bitcoin/bitcoin/issues/20725#issuecomment-1624279055)
> @ismaelsadeeq Can you try with the commit specified in OP as well? If it fails, can you try bisecting?

Maybe not necessary:
There are newer crashes reported in #23165 (which seems to be the same problem). The seed `7288975409214300634` from https://api.cirrus-ci.com/v1/task/6110951150190592/logs/ci.log leads to a crash on current master (the test passes with #21161 rebased on master).

💬 achow101 commented on pull request "wallet: address book migration bug fixes":
(https://github.com/bitcoin/bitcoin/pull/28038#issuecomment-1624292395)
ACK 7ecc29a0b7a23d8f5d3c1e6a0dad29b3ad839eb9

Nice catch!

🚀 ryanofsky merged a pull request: "kernel: Rm ShutdownRequested and AbortNode from validation code."
(https://github.com/bitcoin/bitcoin/pull/27861)

💬 achow101 commented on pull request "wallet: sqlite: don't include sqlite files from our headers":
(https://github.com/bitcoin/bitcoin/pull/28040#issuecomment-1624315734)
ACK bea9fc2600635020fd28ec7a6613c92a6f349a86

💬 furszy commented on pull request "index: make startup more efficient":
(https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1254925898)
Sure, applied. Thanks!

💬 sipa commented on pull request "Add feerate histogram to getmempoolinfo":
(https://github.com/bitcoin/bitcoin/pull/21422#issuecomment-1624321895)
@santochibtc "Feerate histogram" here is a bytes vs fee diagram, not a #tx vs fee diagram.

🤔 furszy reviewed a pull request: "index: make startup more efficient"
(https://github.com/bitcoin/bitcoin/pull/27607#pullrequestreview-1517325528)
Updated per feedback, thanks. [Small diff](https://github.com/bitcoin/bitcoin/compare/30b2511f39d32e29f9f05859aa8a97b84c22376b..988f3f692acabf0eedfed38e4704fe355f995e72).

💬 furszy commented on pull request "index: make startup more efficient":
(https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1254926452)
done

💬 furszy commented on pull request "index: make startup more efficient":
(https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1254926965)
removed.

💬 achow101 commented on pull request "wallet: don't include bdb files from our headers":
(https://github.com/bitcoin/bitcoin/pull/28039#issuecomment-1624325625)
ACK 2d09d0f50408f9c522b9efa4f386072502c7b3d1

Changes are primarily moves and appear otherwise obviously correct.

💬 mzumsande commented on issue "qa: Intermittent failure in `feature_fee_estimation.py`":
(https://github.com/bitcoin/bitcoin/issues/23165#issuecomment-1624334379)
Just noting here too (I also posted in #20725) that failures can be reproduced by using the PRNG seed from the log (`7288975409214300634` in the most recent one) and running with `--randomseed`. So it would be good to include that seed in future reports - the earlier ones are no longer accessible from cirrus.

💬 jonatack commented on pull request "p2p: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1254974082)
Thanks, makes sense, and I see that your change makes the conditional here the same as in `ProcessMessage::TX` followed by the same comment.

```cpp
if (peer->m_wtxid_relay && txid != wtxid) {
// Insert txid into m_tx_inventory_known_filter, even for
// wtxidrelay peers. This prevents re-adding of
// unconfirmed parents to the recently_announced
// filter, when a child tx is requested. See
// ProcessGetData().

...

🤔 ismaelsadeeq reviewed a pull request: "Fee estimation: extend bucket ranges consistently"
(https://github.com/bitcoin/bitcoin/pull/21161#pullrequestreview-1517440130)
The issue this is solving occurs intermittently.
I tested the test failed on recent master c325f0f with [seed](https://github.com/bitcoin/bitcoin/issues/20725#issuecomment-1624279055) from @mzumsande and passed on this PR rebased on master head c325f0f.

Did not finish understanding the PR yet

🤔 jonatack reviewed a pull request: "p2p: Drop m_recently_announced_invs bloom filter"
(https://github.com/bitcoin/bitcoin/pull/27675#pullrequestreview-1517398008)
ACK 172fa815e08b54c61b24d552f065df15046aef3c

With this simplification, test coverage for the new logic might be feasible?

💬 jonatack commented on pull request "p2p: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1254977700)
caf32d2 mentioning `m_sequence_number`'s new responsibility for p2p tx relay might be helpful (or alternatively, drop "external")

```suggestion
// In-memory counter for internal (p2p tx relay) and external (zmq) mempool tracking purposes.
```

💬 jonatack commented on pull request "p2p: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1254981645)
c219b68 nit, could add `[[nodiscard]]` while touching this

💬 jonatack commented on pull request "p2p: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1254980474)
caf32d2 nit, could be ` [[nodiscard]]`

👍 theStack approved a pull request: "Add support for RFC8439 variant of ChaCha20"
(https://github.com/bitcoin/bitcoin/pull/27985#pullrequestreview-1517469616)
Code-review ACK 200e1a292b277d6c32dbd6081d38b715cbc079cb

Verified that the implementation matches the RFC8439 specification (https://datatracker.ietf.org/doc/html/rfc8439#section-2.3) and checked that the test vectors match the ones from the linked sources (https://tools.ietf.org/html/draft-agl-tls-chacha20poly1305-04#section-7 and https://datatracker.ietf.org/doc/html/rfc8439#page-30).

Note for other reviewers (that get as easily confused as me): we don't store the constants in the `inp
...

💬 achow101 commented on pull request "Add support for RFC8439 variant of ChaCha20":
(https://github.com/bitcoin/bitcoin/pull/27985#issuecomment-1624433876)
Is the current ChaCha20 implementation used for anything that needs backwards compatibility?

💬 sipa commented on pull request "Add support for RFC8439 variant of ChaCha20":
(https://github.com/bitcoin/bitcoin/pull/27985#issuecomment-1624469014)
@achow101 Good question!

It's used by FastRandomContext and MuHash. I haven't investigated whether there are any uses of FastRandomContext that could generate more than 256 GiB of data, but even if there are there it's possible to at least use the new seeking style (the current implementation, when using the 96/32 split, when the block counter overflows, just increments the nonce, which is exactly what we'd want for an RNG anyway).

So no - I think all existing uses could be converted to us
...