💬 satsie commented on pull request "rpc: add 'getnetmsgstats', new rpc to view network message statistics":
(https://github.com/bitcoin/bitcoin/pull/27534#discussion_r1190350382)
Ooooo thank you! C++ rookie here :smile:

Fixed in f5c8b5f78f93bea96e6846ef9d5b787269029b50

🤔 mzumsande reviewed a pull request: "refactor, kernel: Decouple ArgsManager from blockstorage"
(https://github.com/bitcoin/bitcoin/pull/27125#pullrequestreview-1421312759)
Code Review ACK 5ff63a09a9edd1204b2cc56cf6f48a44adab7bb3

💬 mzumsande commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1190350876)
nit (if you repush): blockman should be added to the doc, also it would be nice to keep the out param at the end according to dev notes.

💬 satsie commented on pull request "rpc: add 'getnetmsgstats', new rpc to view network message statistics":
(https://github.com/bitcoin/bitcoin/pull/27534#discussion_r1190362272)
This is a great point. I think I need to take a closer look at some of the refactoring that's going on in `net` in hopes of finding other examples/decisions that support making `NetStats` a member variable in `CNode`. I think the comment you linked to is very relevant.

💬 satsie commented on pull request "rpc: add 'getnetmsgstats', new rpc to view network message statistics":
(https://github.com/bitcoin/bitcoin/pull/27534#issuecomment-1542767255)
Thank you for testing and taking a peek at this @ccdle12 :hugs: Appreciate the review and will definitely be taking you up on the offer for a more in depth review as this evolves! :wink:

💬 achow101 commented on pull request "util: improve FindByte() performance":
(https://github.com/bitcoin/bitcoin/pull/19690#issuecomment-1542843029)
re-ACK 72efc26439da9a1344a19569fb0cab01f82ae7d1

🚀 achow101 merged a pull request: "util: improve FindByte() performance"
(https://github.com/bitcoin/bitcoin/pull/19690)

💬 glozow commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1190437947)
Re-added

💬 glozow commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1190437970)
Yeah. Added some docs to the class (also mentioned BIP331) so maybe it's more clear? I used to call it `Packageifier` because it can potentially build a package out of any random list of transactions. But then it's weird because we packageify a `Package`. Open to naming improvements 😅

💬 glozow commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1190438085)
Elaborated on the comment, hopefully it's better now

💬 glozow commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1190438226)
> I think hashing these bytes would defeat this since it can't generate a valid tx with that txid?

Did this one, thanks 👍

💬 glozow commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1190438295)
Elaborated

💬 glozow commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1190438618)
Changed to always use `package[i]`, and added a comment about this unstable sort

💬 glozow commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1190438675)
Aha, thanks!

📝 ismaelsadeeq opened a pull request: "Fee estimation: avoid serving stale fee estimate"
(https://github.com/bitcoin/bitcoin/pull/27622)
Fixes #27555

The issue arises during initialization where an old `fee_estimates.dat` file is sometimes read during initialization.
Or after an unclean shutdown, the latest fee estimates are not flushed to `fee_estimates.dat`.
If the fee estimates in the old file are old, they can cause transactions to become stuck in the mempool.
This PR ensures that nodes do not use stale estimates from the old file during initialization. If `fee_estimates.dat`
has not been updated for 12 hours or mo
...

💬 theuni commented on pull request "build: LLVM 16 & LLD based macOS toolchain":
(https://github.com/bitcoin/bitcoin/pull/21778#issuecomment-1542911418)
I've experimented and implemented with the above here: https://github.com/theuni/bitcoin/commits/21778

@fanquake I think we want https://github.com/theuni/bitcoin/commit/a0427fc986294ef81605b4c44b54c3ec9825bf48 regardless, which fixes the libbitcoinconsensus linking issue. tl;dr: It makes lld linking work like ld64 by skipping a busted test. The `-r` was a red herring as a result of that busted test.

Shall I go ahead and PR that commit separately?

💬 ajtowns 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#discussion_r1190497125)
`MAX_PEER_TX_ANNOUNCEMENTS` doesn't really provide a rate limit -- it stops the queue from being more than 5k at any point in time, but the flow through the queue could still be 400tx/s, for instance. Beyond that, `m_recently_announced_invs` only tracks things we announced, not things we received, and we rate limit that (as per the static_assert on `INVENTORY_MAX_RECENT_RELAY`).

Delaying MEMPOOL results for 2 minutes seems annoying. But how about just recording the timestamp of the last MEMP
...

💬 ajtowns commented on pull request "Improve performance of p2p inv to send queues":
(https://github.com/bitcoin/bitcoin/pull/27610#discussion_r1190528963)
The `3500` figure is calculated based on outbound peers, which already have 2.5x the INV rate of inbound peers. An inbound peer would need to be at 65 or 70 txids per INV message to have the same 1-in-a-million chance of a problem, implying a tx rate of 13 or 14 tx/s. To get to a 1-in-100 chance of a problem, you'd need to hit 90-to-95 txids per INV (and sustain that for ~40 INVs, and hit the 1-in-100 chance of sending those 40 INVs in a 2 minute period). Those seem okay to me.

💬 ajtowns commented on pull request "Improve performance of p2p inv to send queues":
(https://github.com/bitcoin/bitcoin/pull/27610#discussion_r1190533182)
Just seems like more code to do it that way, since that is just dealing with txids and `CompareDepthAndScore` already has access to the mempool info.

💬 ajtowns commented on pull request "Improve performance of p2p inv to send queues":
(https://github.com/bitcoin/bitcoin/pull/27610#discussion_r1190574243)
It's more related to `MAX_PEER_TX_ANNOUNCEMENTS=5000` (we don't want to send more than we would be willing to receive) and `INVENTORY_MAX_RECENT_RELAY=3500` (we don't want to advertise txids and then refuse to relay them because we've forgotten that we advertised them).

(Could make sense to limit it to 90 for inbounds and 45 for outbounds as (if sustained for two minutes) those values would hit roughly a 1-in-100 chance of overflowing `INVENTORY_MAX_RECENT_RELAY`)