💬 ajtowns commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1227472166)
I don't think this needs a diamond, even.

For example: mempool is evicting below 10sat/vb, grandparent is A at 3300sat, 100vb (33sat/vb); parent is B spending A, at 700sat, 200vb (3.5sat/vb), child is C, spending B at 2000 sat, 100vb (20sat/vb). If you accept A first, then B alone is still below the eviction threshold, but so is B+C (2700sat, 300vb 9sat/vb), even though C is doing cpfp here.

💬 ajtowns commented on pull request "validation: Replace MinBIP9WarningHeight with MinBIP9WarningStartTime":
(https://github.com/bitcoin/bitcoin/pull/27427#discussion_r1227490711)
It's more that `MinBIP9WarningHeight` might be lower, 0 in particular.

💬 hebasto commented on pull request "Use `int32_t` type for most transaction size/weight values":
(https://github.com/bitcoin/bitcoin/pull/23962#issuecomment-1588603011)
The silent conflict with the https://github.com/bitcoin/bitcoin/pull/26531 has been resolved in the recent push. As this PR touches tracepoints now, @0xB10C @virtu maybe you want to look into it?

> Something that just occurred to me: do/should we consider tracepoints to be a stable api?

No, we shouldn't. I think that classes like `CTxMemPoolEntry` and their public access method return types should be considered as mempool implementation details for all purposes. Maybe tracepoint docs need
...

💬 MarcoFalke commented on pull request "test: fix intermittent failure in p2p_leak_tx.py":
(https://github.com/bitcoin/bitcoin/pull/27864#issuecomment-1588656226)
Thanks!

lgtm ACK ee2417ed614d6a298f932ac068702ab2abee3cdf

💬 MarcoFalke commented on pull request "net: introduce block tracker to retry to download blocks after failure":
(https://github.com/bitcoin/bitcoin/pull/27837#issuecomment-1588705491)
Maybe mark as draft if CI is red and this is still based on something else?

💬 dimitaracev commented on pull request "wallet: Give deprecation warning when loading a legacy wallet":
(https://github.com/bitcoin/bitcoin/pull/27869#issuecomment-1588723504)
ACK [956d05b](https://github.com/bitcoin/bitcoin/pull/27869/commits/956d05bad21478a110a0e798cb2c4b69de62a8bb)

👍 Dezzj approved a pull request: "ci: enable AArch64 target in MSAN jobs"
(https://github.com/bitcoin/bitcoin/pull/27824#pullrequestreview-1476494235)
Approved

💬 hebasto commented on pull request "Use `int32_t` type for most transaction size/weight values":
(https://github.com/bitcoin/bitcoin/pull/23962#discussion_r1227719150)
> Are we actually bumping into overflow issues with `int32_t` somewhere for size with descendants?

I don't think so.

However, UBSan raises "signed-integer-overflow" warnings about `nSizeWithAncestors += modifySize;` etc.

📝 fanquake converted_to_draft a pull request: "net: introduce block tracker to retry to download blocks after failure"
(https://github.com/bitcoin/bitcoin/pull/27837)
Built on top of #27836. Coming from #27652. Implementing the second part of it.

The general idea is to keep track of the user requested blocks so, in
case of a bad behaving peer or a network disconnection, they can be
fetched from another one automatically without any further user interaction.

This was requested by users because the `getblockfrompeer` RPC command
lacks the functionality to notify them about block request failures or peer
disconnections (which is expected due to the asy
...

💬 hebasto commented on pull request "ci: Use docker image cache for "Win64 native [vs2022]" task":
(https://github.com/bitcoin/bitcoin/pull/27771#issuecomment-1588809598)
Friendly ping @MarcoFalke and @sipsorcery :)

✅ fanquake closed an issue: "tsan: failure in p2p_leak_tx.py"
(https://github.com/bitcoin/bitcoin/issues/27860)

🚀 fanquake merged a pull request: "test: fix intermittent failure in p2p_leak_tx.py"
(https://github.com/bitcoin/bitcoin/pull/27864)

💬 MarcoFalke commented on pull request "ci: Use docker image cache for "Win64 native [vs2022]" task":
(https://github.com/bitcoin/bitcoin/pull/27771#issuecomment-1588852287)
No objection to using the cache to avoid intermittent network issues. But the speedup was only ~2 minutes on a total runtime of 1 hour?

💬 hebasto commented on pull request "ci: Use docker image cache for "Win64 native [vs2022]" task":
(https://github.com/bitcoin/bitcoin/pull/27771#issuecomment-1588861202)
> No objection to using the cache to avoid intermittent network issues. But the speedup was only ~2 minutes on a total runtime of 1 hour?

You're right. I've added "avoid intermittent network issues" to the PR description.

💬 MarcoFalke commented on pull request "test: Add unit & functional test coverage for blockstore":
(https://github.com/bitcoin/bitcoin/pull/27850#discussion_r1227790565)
shouldn't the messages by checked by `assert_start_raises_init_error` instead of reading the debug log?

🤔 MarcoFalke reviewed a pull request: "Return EXIT_FAILURE on post-init fatal errors"
(https://github.com/bitcoin/bitcoin/pull/27708#pullrequestreview-1476651234)
left a question / test nit for follow-up

💬 MarcoFalke commented on pull request "Return EXIT_FAILURE on post-init fatal errors":
(https://github.com/bitcoin/bitcoin/pull/27708#discussion_r1227794716)
In 3b2c61e8198bcefb1c2343caff1d705951026cc4:

Any reason to make `expected_ret_code` optional? Seems easier to just make the default value `0` and drop this whole `else` block.

💬 ismaelsadeeq commented on pull request "Mempool: persist mempoolminfee accross restarts":
(https://github.com/bitcoin/bitcoin/pull/27859#discussion_r1227810641)
Thank you. Yes, I agree that adding it at the end of `fee_estimates.dat` would indeed reduce redundancy. I would update that.

Regarding making it conditional to read the persisted `mempoolminfee`, could you please clarify the scenarios in which we might want to use the default `mempoolminfee` value? In this, it is already conditional to remember the `mempoolminfee` as stale `mempoolminfee` will not be read at all.

💬 MarcoFalke commented on pull request "fuzz: Fix mini_miner_selection running out of coin":
(https://github.com/bitcoin/bitcoin/pull/27806#issuecomment-1588895527)
lgtm ACK 76c5ea703e77d580b6962e60398f4988cbd9b58b

💬 furszy commented on pull request "net: introduce block tracker to retry to download blocks after failure":
(https://github.com/bitcoin/bitcoin/pull/27837#issuecomment-1588916036)
Oh, didn't know about the CI failure. Seems to be just an unused variable. But sure, moved to draft until https://github.com/bitcoin/bitcoin/pull/27836 get merged.

💬 ismaelsadeeq commented on pull request "Mempool: persist mempoolminfee accross restarts":
(https://github.com/bitcoin/bitcoin/pull/27859#discussion_r1227846644)
It appears that taking the approach of dumping to `fee_estimates.dat` and implementing a method to set `mempoolminfee` based on the persisted value is a better option.

Your perspective on persisting the mempool periodically. I understand the concern about remembering the fee estimates and `mempoolminfee` without the mempool itself. However, I would like to seek further insight into the potential consequences that could arise from this, as dumping the mempool periodically might be unappealing