💬 glozow commented on pull request "refactors for subpackage evaluation":
(https://github.com/bitcoin/bitcoin/pull/28758#discussion_r1379095337)
ok

💬 glozow commented on pull request "refactors for subpackage evaluation":
(https://github.com/bitcoin/bitcoin/pull/28758#discussion_r1379095637)
fixed

💬 glozow commented on pull request "refactors for subpackage evaluation":
(https://github.com/bitcoin/bitcoin/pull/28758#discussion_r1379095561)
I made it iswellformedpackage

💬 TheCharlatan commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1379219584)
Yes, as I understand it that is what is checked in the code block above, so this should always hold.

💬 TheCharlatan commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1379220432)
Huh, yeah. That is more straight forward.

💬 TheCharlatan commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#issuecomment-1789568785)
Thank you for the review @ismaelsadeeq!

Updated e579bb98ba8977af284ba6914ffb2b1da7f34cdd -> 105a0f4db4ffdc25d3ad30300c949d46d5d8e647 ([simplifyMemPoolInteractions_3](https://github.com/TheCharlatan/bitcoin/tree/simplifyMemPoolInteractions_3) -> [simplifyMemPoolInteractions_4](https://github.com/TheCharlatan/bitcoin/tree/simplifyMemPoolInteractions_4), [compare](https://github.com/TheCharlatan/bitcoin/compare/simplifyMemPoolInteractions_3..simplifyMemPoolInteractions_4))

* Added commit 105
...

💬 maflcko commented on pull request "p2p: peer connection bug fixes":
(https://github.com/bitcoin/bitcoin/pull/28248#issuecomment-1789583586)
Maybe mark as draft while CI is red?

💬 TheCharlatan commented on pull request "Fee Estimator updates from Validation Interface/CScheduler thread":
(https://github.com/bitcoin/bitcoin/pull/28368#issuecomment-1789598015)
Seems like with the changes here, the `policy/fees` can be removed the kernel library. If you want to, you can integrate this [one-line commit](https://github.com/TheCharlatan/bitcoin/commit/32846141df1a2e110f84f3244d5e98626d379185), otherwise I'll make a tiny follow up.

💬 jonatack commented on pull request "p2p: peer connection bug fixes":
(https://github.com/bitcoin/bitcoin/pull/28248#issuecomment-1789598397)
Done. Note that only the latest push is red. I can push a version that is green, but have been rebasing on #28155 that I reckon will be merged soon, and adding missing test coverage. Concept ACKs on fixing the bugs would be encouraging 😃

📝 jonatack converted_to_draft a pull request: "p2p: peer connection bug fixes"
(https://github.com/bitcoin/bitcoin/pull/28248)
This pull fixes several peer connection bugs in our p2p code, along with the logging that uncovered them:

1. Fix detection of inbound peer connections in `GetAddedNodeInfo`.

2. Fix addnode CJDNS peers not detected in `GetAddedNodeInfo`, causing `ThreadOpenAddedConnections` to continually retry to connect to them and RPC `getaddednodeinfo` incorrectly showing them as not connected.

3. Fix `ThreadOpenConnections` not detecting inbound CJDNS connections and making automatic outbound connec
...

👍 TheCharlatan approved a pull request: "refactor: Remove unused circular include dependency from validation.cpp"
(https://github.com/bitcoin/bitcoin/pull/28770#pullrequestreview-1709014758)
ACK fa7d31910ab181c7e0e5f1fa1e23a49e208aec2a

💬 jonatack commented on pull request "p2p: peer connection bug fixes":
(https://github.com/bitcoin/bitcoin/pull/28248#issuecomment-1789612236)
Note also, that item 7 in the PR description involves a regression in v26.x.

🤔 mzumsande reviewed a pull request: "p2p: Fill reconciliation sets (Erlay)"
(https://github.com/bitcoin/bitcoin/pull/28765#pullrequestreview-1709034021)
Approach ACK

💬 mzumsande commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1379306577)
nit: remove one `//`

💬 mzumsande commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1379306928)
I wonder if this algorithm (which took me a while to fully understand) could be simpler.
E.g., if we used a sorted container for `best_peers` instead of a vector, inserted all of the peers, and then finally return the first `targets` elements of that container, I think we could do without the `try_fanout_candidate` lambda.
Or would that be incorrect / less performant?

I'm thinking of something like the following (just to show idea, I didn't test it):

struct ComparePairs {
b
...

💬 mzumsande commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1379309905)
Do we have to first add and then (maybe) drop a peer here (instead of determining how many peers we want at the beginning, and then getting as many peers as we can up the desired number).

💬 mzumsande commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1379299664)
Could you explain a bit more why this is necessary, what is the lock order that would get violated if we did the locking later?

💬 mzumsande commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1379312851)
3/30 is integer, so maybe also add an example with a fraction. If we run it often enough, we could probably assert that two values for `total_fanouted` are possible.

💬 mzumsande commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1379313360)
nit: remove empty line

💬 fanquake commented on pull request "p2p: peer connection bug fixes":
(https://github.com/bitcoin/bitcoin/pull/28248#issuecomment-1789659513)
> Note also, that item 7 in the PR description involves a regression in v26.x.

What is the regression? Which change caused it?