💬 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
...
(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?
(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.
(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 😃
(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
...
(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
(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.
(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
(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 `//`
(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
...
(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).
(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?
(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.
(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
(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?
(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?
💬 mzumsande commented on pull request "p2p: peer connection bug fixes":
(https://github.com/bitcoin/bitcoin/pull/28248#issuecomment-1789702441)
> Concept ACKs on fixing the bugs would be encouraging 😃
I think that some of the commits are straightforward bugfixes (e.g. the missing `MaybeFlipIPv6toCJDNS` calls or the logging improvements), whereas others are fixing some non-ideal behavior in some cases, but have the risk of introducing new non-ideal behavior in other cases because the fix comes with downsides of its own.
For example, if it isn't possible to distinguish situations in which two connections involving the same IP are c
...
(https://github.com/bitcoin/bitcoin/pull/28248#issuecomment-1789702441)
> Concept ACKs on fixing the bugs would be encouraging 😃
I think that some of the commits are straightforward bugfixes (e.g. the missing `MaybeFlipIPv6toCJDNS` calls or the logging improvements), whereas others are fixing some non-ideal behavior in some cases, but have the risk of introducing new non-ideal behavior in other cases because the fix comes with downsides of its own.
For example, if it isn't possible to distinguish situations in which two connections involving the same IP are c
...
💬 maflcko commented on pull request "suppressions: note that `type:ClassName::MethodName` should be used":
(https://github.com/bitcoin/bitcoin/pull/28147#issuecomment-1789703076)
Not sure why the symbolizer doesn't work on Fedora, but I guess this should be reverted? Steps to reproduce on a fresh install of Fedora 39:
```
dnf install gcc-c++ libtool make autoconf automake python3 clang llvm libcxx-devel libcxxabi-devel git ccache libevent-devel boost-devel -y && git clone https://github.com/bitcoin/bitcoin.git --depth=1 ./bitcoin-core && cd bitcoin-core && ./autogen.sh && ./configure CC=clang CXX=clang++ --with-sanitizers=fuzzer,integer,undefined --enable-fuzz
...
(https://github.com/bitcoin/bitcoin/pull/28147#issuecomment-1789703076)
Not sure why the symbolizer doesn't work on Fedora, but I guess this should be reverted? Steps to reproduce on a fresh install of Fedora 39:
```
dnf install gcc-c++ libtool make autoconf automake python3 clang llvm libcxx-devel libcxxabi-devel git ccache libevent-devel boost-devel -y && git clone https://github.com/bitcoin/bitcoin.git --depth=1 ./bitcoin-core && cd bitcoin-core && ./autogen.sh && ./configure CC=clang CXX=clang++ --with-sanitizers=fuzzer,integer,undefined --enable-fuzz
...
💬 jonatack commented on pull request "p2p: peer connection bug fixes":
(https://github.com/bitcoin/bitcoin/pull/28248#issuecomment-1789704725)
Reported in https://github.com/bitcoin/bitcoin/pull/27213/files#r1291926369 and described in https://github.com/bitcoin/bitcoin/pull/28248/commits/21f8426c823fd83c40fd81f08ff2bf39ec6a4575, there is an interaction between the new network-specific automatic outbound logic and the addnode list: the former doesn't account for the latter.
```markdown
Notable changes
===============
P2P and network changes
-----------------------
- Nodes with multiple reachable networks will actively try
...
(https://github.com/bitcoin/bitcoin/pull/28248#issuecomment-1789704725)
Reported in https://github.com/bitcoin/bitcoin/pull/27213/files#r1291926369 and described in https://github.com/bitcoin/bitcoin/pull/28248/commits/21f8426c823fd83c40fd81f08ff2bf39ec6a4575, there is an interaction between the new network-specific automatic outbound logic and the addnode list: the former doesn't account for the latter.
```markdown
Notable changes
===============
P2P and network changes
-----------------------
- Nodes with multiple reachable networks will actively try
...
💬 jonatack commented on pull request "p2p: peer connection bug fixes":
(https://github.com/bitcoin/bitcoin/pull/28248#issuecomment-1789710867)
> Maybe some of the straightforward bugfixes could be split out into another PR?
Yes -- looking at doing that, or dropping here for now any that aren't relatively clear.
(https://github.com/bitcoin/bitcoin/pull/28248#issuecomment-1789710867)
> Maybe some of the straightforward bugfixes could be split out into another PR?
Yes -- looking at doing that, or dropping here for now any that aren't relatively clear.
💬 jonatack commented on pull request "p2p: peer connection bug fixes":
(https://github.com/bitcoin/bitcoin/pull/28248#discussion_r1379352136)
Yes, I've been running this branch with its logging on for several months, and grepping the log turns up dozens, sometimes hundreds, of connection attempts per day from one or two inbound I2P peers on a given day that are already connected. I'm not sure at the moment of writing whether the behavior is malicious or due to buggy outbound logic (which would only be fixed in future releases and not existing releases connecting to us, though) and am drilling down more, but it seems like a good idea n
...
(https://github.com/bitcoin/bitcoin/pull/28248#discussion_r1379352136)
Yes, I've been running this branch with its logging on for several months, and grepping the log turns up dozens, sometimes hundreds, of connection attempts per day from one or two inbound I2P peers on a given day that are already connected. I'm not sure at the moment of writing whether the behavior is malicious or due to buggy outbound logic (which would only be fixed in future releases and not existing releases connecting to us, though) and am drilling down more, but it seems like a good idea n
...