💬 stratospher commented on pull request "rpc: Add test-only RPC getaddrmaninfo for new/tried table address count":
(https://github.com/bitcoin/bitcoin/pull/27511#issuecomment-1548177296)
so sorry for the delay and thank you for the suggestions! the CI is all green now. i've updated the PR to include:
- addrman new/tried/total count for "all networks"
- test coverage for "all networks" and for total (total refers to both new/tried table count)

I'd want this RPC to be the alternative way to access the actual count of all addresses in the addrman.

💬 instagibbs commented on pull request "rpc: allow submitpackage to be called outside of regtest":
(https://github.com/bitcoin/bitcoin/pull/27609#issuecomment-1548180934)
@glozow ok so the tree thing is a bandaid in lieu of #26711 , and you'd rather just to that

@joostjager

> If I understood the various threads here and in other prs correctly, I think the consequence of this is that the mempool will be sub-optimal and lead to a block template that earns fewer fees for the miner exposing the endpoint?

Also can have weird effects like your mempool min rate going higher than it should normally.

I think we can easily get #26711 in next release

💬 furszy commented on pull request "wallet: fix deadlock in bdb read write operation":
(https://github.com/bitcoin/bitcoin/pull/27556#issuecomment-1548189712)
rebased, conflicts solved.

💬 Sjors commented on pull request "Improve performance of p2p inv to send queues":
(https://github.com/bitcoin/bitcoin/pull/27610#discussion_r1194086866)
@dergoegge `INVENTORY_BROADCAST_MAX` has been poorly named for a while. It applies to inbound peers, but to outbounds we send 5 / 2 more.

💬 brunoerg commented on pull request "fuzz: wallet, add target for `fees`":
(https://github.com/bitcoin/bitcoin/pull/27647#issuecomment-1548198822)
CI failure seems unrelated

💬 joostjager commented on pull request "rpc: allow submitpackage to be called outside of regtest":
(https://github.com/bitcoin/bitcoin/pull/27609#issuecomment-1548199779)
>Also can have weird effects like your mempool min rate going higher than it should normally.

Is this something that #26711 prevents? Because it seems to me that that PR is less restrictive than the tree-only allowance in this PR.

💬 instagibbs commented on pull request "rpc: allow submitpackage to be called outside of regtest":
(https://github.com/bitcoin/bitcoin/pull/27609#issuecomment-1548203219)
To be clear, the tree structure in this PR "should" also stop it.

💬 joostjager commented on pull request "rpc: allow submitpackage to be called outside of regtest":
(https://github.com/bitcoin/bitcoin/pull/27609#issuecomment-1548207594)
By the way, about the diamond example in https://github.com/bitcoin/bitcoin/pull/27609#issuecomment-1542695316

This isn't a "child with parents" package and also wouldn't be accepted on regtest with the current master branch because the child has no direct link to the grandparent? (https://github.com/bitcoin/bitcoin/blob/d7700d3a26478d9b1648463c188648c7047b1c60/src/policy/packages.cpp#L68)

💬 brunoerg commented on pull request "p2p: Allow whitelisting outgoing connections":
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1194103566)
Make sense, going to address it.

💬 glozow commented on pull request "rpc: allow submitpackage to be called outside of regtest":
(https://github.com/bitcoin/bitcoin/pull/27609#issuecomment-1548213087)
> By the way, about the diamond example in https://github.com/bitcoin/bitcoin/pull/27609#issuecomment-1542695316

> This isn't a "child with parents" package

Sorry, that was a bad diagram (of the unit test I had added). The child is also spending the grandparent. I've updated the comment now.

💬 stratospher commented on pull request "cli: rework -addrinfo cli to use addresses which aren’t filtered for quality/recency":
(https://github.com/bitcoin/bitcoin/pull/26988#discussion_r1194105588)
noted. not convinced how desirable the additional code complexity is. would like to hear how other people think about code complexity too before deciding what to do next.

💬 stratospher commented on pull request "cli: rework -addrinfo cli to use addresses which aren’t filtered for quality/recency":
(https://github.com/bitcoin/bitcoin/pull/26988#issuecomment-1548215577)
Rebased. Looking for feedback on [this comment](https://github.com/bitcoin/bitcoin/pull/26988#discussion_r1173966799).

💬 joostjager commented on pull request "rpc: allow submitpackage to be called outside of regtest":
(https://github.com/bitcoin/bitcoin/pull/27609#issuecomment-1548224928)
It is still not completely clear to me what the risk is with merging this PR if #27611 is less restrictive. It would be great if it makes the next release, but also it is another six months delay for this important step.

💬 brunoerg commented on pull request "p2p: Allow whitelisting outgoing connections":
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1194120554)
I think it was accidental, sorry. I will revert it.

💬 brunoerg commented on pull request "p2p: Allow whitelisting outgoing connections":
(https://github.com/bitcoin/bitcoin/pull/27114#issuecomment-1548238840)
Force pushed addressing:
- https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1191183121
- https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1191191757
- https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1191197012
- https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1191201370

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

💬 instagibbs commented on pull request "rpc: allow submitpackage to be called outside of regtest":
(https://github.com/bitcoin/bitcoin/pull/27609#issuecomment-1548247127)
If we're not shooting for a backport for 25.x, I don't think the time difference is meaningful? This PR is adding stuff that will directly be removed and is sort of a distraction for reviewers themselves.

💬 ryanofsky commented on pull request "init: verify blocks data existence only once for all the indexers":
(https://github.com/bitcoin/bitcoin/pull/27607#issuecomment-1548248095)
re: https://github.com/bitcoin/bitcoin/pull/27607#issuecomment-1544648748

> We could also move the entire `hasDataFromTipDown` from the interface to somewhere else in this way. So there is no `CBlockIndex` dependency in the interface neither here nor in the #24230 intermediate commits.

Yes I don't think it makes sense to expose that method as part of the `Chain` interface, and even if it did make sense, it wouldn't make sense to put the implementation in the `ChainImpl` class, because the
...

💬 Sjors commented on pull request "Improve performance of p2p inv to send queues":
(https://github.com/bitcoin/bitcoin/pull/27610#discussion_r1194135762)
Note to others: we may send outbound peers 7 * 2.5 * 60 * 2 ~= 2100 transaction ids per 2 minutes. Depending on random variation it can be more. `UNCONDITIONAL_RELAY_DELAY = 2min;` is the period of time that we have to remember what we sent to a specific peer. For privacy reason we don't want to reveal these transactions to _other_ (spy) peers yet. We do this tracking with a bloom filter `m_recently_announced_invs` that has a 1 in a million chance of messing up. I forgot if it's false positive (
...

💬 TheCharlatan commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1194151828)
Thank you for the time you took for these detailed comments. Fleshing this out to a more proper kernel interface sounds good to me. How about moving `ValidationNotifications` to the `KernelContext` instead? Or does that conflict with the vision you have for getting rid of `uiInterface` global?

💬 ryanofsky commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1194168060)
> Thank you for the time you took for these detailed comments. Fleshing this out to a more proper kernel interface sounds good to me. How about moving `ValidationNotifications` to the `KernelContext` instead? Or does that conflict with the vision you have for getting rid of `uiInterface` global?

Hmm, It doesn't make sense to me that an object which is supposed to receive notifications from the kernel would be owned by the kernel. Outside code that wants to receive notifications should be able
...