💬 ccdle12 commented on pull request "net, refactor: extract Network and BIP155Network logic to node/network":
(https://github.com/bitcoin/bitcoin/pull/27385#issuecomment-1551632239)
tACK for creating smaller compilation units.
I'm just curious, for the motivation of future changes, should there be more node networking related logic moved to the node sub folder (e.g. logic related to the node making decisions on networking but not the actual networking implementations themselves) or is it simply classes/enums that are called frequently in isolation, in different parts of the code base but they exist in bigger files with unused imports like in `netbase` and `netaddress`?
...
(https://github.com/bitcoin/bitcoin/pull/27385#issuecomment-1551632239)
tACK for creating smaller compilation units.
I'm just curious, for the motivation of future changes, should there be more node networking related logic moved to the node sub folder (e.g. logic related to the node making decisions on networking but not the actual networking implementations themselves) or is it simply classes/enums that are called frequently in isolation, in different parts of the code base but they exist in bigger files with unused imports like in `netbase` and `netaddress`?
...
🤔 sdaftuar reviewed a pull request: "Parallel compact block downloads, take 3"
(https://github.com/bitcoin/bitcoin/pull/27626#pullrequestreview-1430785565)
I have not reviewed the test or done any manual testing yet, but just finished reading the code.
One conceptual thing that I wanted to mention: right now it is possible for none of our HB compactblock peers to be outbound. The way the HB outbound reservation logic works is that if we ever have an outbound peer taking an HB slot, we will not allow inbounds to take over all 3 slots. However, it is possible that a peer starting up for the first time could have all its slots taken by inbounds.
(https://github.com/bitcoin/bitcoin/pull/27626#pullrequestreview-1430785565)
I have not reviewed the test or done any manual testing yet, but just finished reading the code.
One conceptual thing that I wanted to mention: right now it is possible for none of our HB compactblock peers to be outbound. The way the HB outbound reservation logic works is that if we ever have an outbound peer taking an HB slot, we will not allow inbounds to take over all 3 slots. However, it is possible that a peer starting up for the first time could have all its slots taken by inbounds.
💬 sdaftuar commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1196575479)
nit: This comment is not right, should say something like "was not requested from any peer".
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1196575479)
nit: This comment is not right, should say something like "was not requested from any peer".
💬 sdaftuar commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1196713707)
So if we get 3 compact block announcements from 3 peers, and we then send 3 getblocktxn messages to them, and then the first blocktxn comes back and reconstruction fails, we will wait until we get the third blocktxn message back before we send out a getdata message for the full block, I think?
That seems suboptimal, though I don't have a fix in mind yet.
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1196713707)
So if we get 3 compact block announcements from 3 peers, and we then send 3 getblocktxn messages to them, and then the first blocktxn comes back and reconstruction fails, we will wait until we get the third blocktxn message back before we send out a getdata message for the full block, I think?
That seems suboptimal, though I don't have a fix in mind yet.
💬 sdaftuar commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1196640876)
Doesn't this line need to be removed here?
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1196640876)
Doesn't this line need to be removed here?
💬 sdaftuar commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1196669983)
What's the justification for this change? It seems like this changes things so that in edge-case scenarios, we wouldn't allow the opportunistic compact block reconstruction to take place, and I don't see why we need to do that.
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1196669983)
What's the justification for this change? It seems like this changes things so that in edge-case scenarios, we wouldn't allow the opportunistic compact block reconstruction to take place, and I don't see why we need to do that.
💬 sdaftuar commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1196679410)
nit: perhaps "We should not have requested too many of this block"
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1196679410)
nit: perhaps "We should not have requested too many of this block"
💬 sdaftuar commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1196715533)
nit: this comment doesn't seem right
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1196715533)
nit: this comment doesn't seem right
💬 sdaftuar commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1196683873)
Is it worth adding an `Assume()` here that we have strictly less than MAX_CMPCTBLOCKS_INFLIGHT_PER_BLOCK requests outstanding?
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1196683873)
Is it worth adding an `Assume()` here that we have strictly less than MAX_CMPCTBLOCKS_INFLIGHT_PER_BLOCK requests outstanding?
💬 sdaftuar commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1196732363)
This notion of `first_in_flight` is not correct, I think. Imagine we get a headers announcement from an outbound (non-hb) peer, and so we send a getdata(cmpctblock). In the meantime, we get a compact block announcement from an inbound peer which fails to reconstruct. Then the compact block comes back from the outbound peer; `first_in_flight` will be false because we think we have two requests outstanding.
Further down, when we decide whether to send a GETBLOCKTXN message to this non-HB out
...
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1196732363)
This notion of `first_in_flight` is not correct, I think. Imagine we get a headers announcement from an outbound (non-hb) peer, and so we send a getdata(cmpctblock). In the meantime, we get a compact block announcement from an inbound peer which fails to reconstruct. Then the compact block comes back from the outbound peer; `first_in_flight` will be false because we think we have two requests outstanding.
Further down, when we decide whether to send a GETBLOCKTXN message to this non-HB out
...
💬 ddykeman1 commented on issue "Mac osx 12.6.5 ":
(https://github.com/bitcoin/bitcoin/issues/27681#issuecomment-1551672136)
Translated Report (Full Report Below)
-------------------------------------
Process: Bitcoin-Qt [844]
Path: /Applications/Bitcoin-Qt.app/Contents/MacOS/Bitcoin-Qt
Identifier: org.bitcoinfoundation.Bitcoin-Qt
Version: 24.0.1 (24.0.1)
Code Type: X86-64 (Native)
Parent Process: launchd [1]
User ID: 501
Date/Time: 2023-05-17 11:26:06.6243 -0400
OS Version: macOS 12.6.5 (21G53
...
(https://github.com/bitcoin/bitcoin/issues/27681#issuecomment-1551672136)
Translated Report (Full Report Below)
-------------------------------------
Process: Bitcoin-Qt [844]
Path: /Applications/Bitcoin-Qt.app/Contents/MacOS/Bitcoin-Qt
Identifier: org.bitcoinfoundation.Bitcoin-Qt
Version: 24.0.1 (24.0.1)
Code Type: X86-64 (Native)
Parent Process: launchd [1]
User ID: 501
Date/Time: 2023-05-17 11:26:06.6243 -0400
OS Version: macOS 12.6.5 (21G53
...
💬 instagibbs commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#issuecomment-1551697808)
> However, it is possible that a peer starting up for the first time could have all its slots taken by inbounds.
Yeah, goes without saying, but maybe it doesn't. This PR offers no protection on fresh restart(no hb peers), and only offers 3 parallel downloads when an outbound is selected as hb and offers a block
(https://github.com/bitcoin/bitcoin/pull/27626#issuecomment-1551697808)
> However, it is possible that a peer starting up for the first time could have all its slots taken by inbounds.
Yeah, goes without saying, but maybe it doesn't. This PR offers no protection on fresh restart(no hb peers), and only offers 3 parallel downloads when an outbound is selected as hb and offers a block
💬 sdaftuar commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1196773469)
Actually, can we use the insertion order guarantees of multimap to solve this?
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1196773469)
Actually, can we use the insertion order guarantees of multimap to solve this?
💬 sdaftuar commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1196774969)
Similarly here, can we use the insertion order of multimap to determine which peer was actually first to make the announcement? If in the situation where an outbound (non-hb) peer is the first to announce that we still always get the block, then this new logic would not be making anything worse, which seems sufficient to me.
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1196774969)
Similarly here, can we use the insertion order of multimap to determine which peer was actually first to make the announcement? If in the situation where an outbound (non-hb) peer is the first to announce that we still always get the block, then this new logic would not be making anything worse, which seems sufficient to me.
💬 TheCharlatan commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1196781453)
Not sure about this one. The include is required for setting the default argument and it is non-const, because we mutate the message.
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1196781453)
Not sure about this one. The include is required for setting the default argument and it is non-const, because we mutate the message.
⚠️ Sataur opened an issue: "Option to prevent sleep"
(https://github.com/bitcoin/bitcoin/issues/27692)
### Please describe the feature you'd like to see added.
There should be option to start Core which enable system to sleep only when net. act. is disabled!
### Is your feature related to a problem, if so please describe it.
_No response_
### Describe the solution you'd like
_To [notify](https://learn.microsoft.com/en-us/windows/win32/power/system-sleep-criteria) the system that your application is busy, use the.._
### Describe any alternatives you've considered
_constant disk activity abo
...
(https://github.com/bitcoin/bitcoin/issues/27692)
### Please describe the feature you'd like to see added.
There should be option to start Core which enable system to sleep only when net. act. is disabled!
### Is your feature related to a problem, if so please describe it.
_No response_
### Describe the solution you'd like
_To [notify](https://learn.microsoft.com/en-us/windows/win32/power/system-sleep-criteria) the system that your application is busy, use the.._
### Describe any alternatives you've considered
_constant disk activity abo
...
💬 instagibbs commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1196823777)
done
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1196823777)
done
💬 instagibbs commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1196823829)
done
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1196823829)
done
💬 instagibbs commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1196823878)
hmm yeah, this is old code, doesn't make sense to me either. removed the change.
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1196823878)
hmm yeah, this is old code, doesn't make sense to me either. removed the change.
💬 instagibbs commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1196823938)
done
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1196823938)
done
💬 instagibbs commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1196823999)
done
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1196823999)
done