💬 willcl-ark commented on pull request "Improve performance of p2p inv to send queues":
(https://github.com/bitcoin/bitcoin/pull/27610#issuecomment-1551582691)
@pinheadmz I am also still running this patch, but I still see pretty stable utilisation in the range of ~2-12%, currently with 88 inbound peers.
(https://github.com/bitcoin/bitcoin/pull/27610#issuecomment-1551582691)
@pinheadmz I am also still running this patch, but I still see pretty stable utilisation in the range of ~2-12%, currently with 88 inbound peers.
💬 willcl-ark commented on issue "Mac osx 12.6.5 ":
(https://github.com/bitcoin/bitcoin/issues/27681#issuecomment-1551598180)
> Please check your debug.log for possible causes; Alternatively you can upload it here.
>
> You can find the debug.log in your [data dir](https://github.com/bitcoin/bitcoin/blob/master/doc/files.md?rgh-link-date=2023-05-17T06%3A16%3A25Z#data-directory-location).
>
> Please be aware that the debug log might contain personally identifying information.
Did you check for the log file in the datadir, as requested by this comment? There should be a file `debug.log` in directory `$HOME/Libr
...
(https://github.com/bitcoin/bitcoin/issues/27681#issuecomment-1551598180)
> Please check your debug.log for possible causes; Alternatively you can upload it here.
>
> You can find the debug.log in your [data dir](https://github.com/bitcoin/bitcoin/blob/master/doc/files.md?rgh-link-date=2023-05-17T06%3A16%3A25Z#data-directory-location).
>
> Please be aware that the debug log might contain personally identifying information.
Did you check for the log file in the datadir, as requested by this comment? There should be a file `debug.log` in directory `$HOME/Libr
...
💬 TheCharlatan commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1196711775)
Mmh, I was originally thinking of moving `util::insert` to `wallet/walletutil.h` and `util::AnyPtr` to `rpc/util.h` in a follow up after this pull request. I think your suggestion is better though. Moving around these kind of headers depending on which part of the code needs them seems silly. I'll patch this then and will apply your other suggestions.
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1196711775)
Mmh, I was originally thinking of moving `util::insert` to `wallet/walletutil.h` and `util::AnyPtr` to `rpc/util.h` in a follow up after this pull request. I think your suggestion is better though. Moving around these kind of headers depending on which part of the code needs them seems silly. I'll patch this then and will apply your other suggestions.
💬 t-bast commented on issue "Proposal for a new mempool design":
(https://github.com/bitcoin/bitcoin/issues/27677#issuecomment-1551620933)
Thanks for sharing all those details, this is great!
I can only comment as someone who relies a lot on mempool/transaction relay for lightning: my opinion may thus be biased.
I believe that lightning's security will *always* depend on the ability to get specific transactions confirmed before a given block, so anything that can help transaction propagation is important for us (and I believe this will be true for most L2 contracts).
> Philosophically, is it problematic for RBF rules to be eve
...
(https://github.com/bitcoin/bitcoin/issues/27677#issuecomment-1551620933)
Thanks for sharing all those details, this is great!
I can only comment as someone who relies a lot on mempool/transaction relay for lightning: my opinion may thus be biased.
I believe that lightning's security will *always* depend on the ability to get specific transactions confirmed before a given block, so anything that can help transaction propagation is important for us (and I believe this will be true for most L2 contracts).
> Philosophically, is it problematic for RBF rules to be eve
...
💬 mzumsande commented on pull request "indexes: Read the locator's top block during init, allow interaction with reindex-chainstate":
(https://github.com/bitcoin/bitcoin/pull/25193#issuecomment-1551628507)
[974140f ](https://github.com/bitcoin/bitcoin/commit/974140f9e721740f857b45d10d7dbab62fdbbe53)to [97844d9](https://github.com/bitcoin/bitcoin/commit/97844d9268b87b5d09b1091bfd0326ed18ce5b91): rebased
> I think it'd be good to just rebase this and merge it and not try to do the "move the indexes threads start after the loading process" change here.
Ok, I only rebased.
@furszy I like your suggestion and will review/test it when you include them in #27607, which I believe will change init
...
(https://github.com/bitcoin/bitcoin/pull/25193#issuecomment-1551628507)
[974140f ](https://github.com/bitcoin/bitcoin/commit/974140f9e721740f857b45d10d7dbab62fdbbe53)to [97844d9](https://github.com/bitcoin/bitcoin/commit/97844d9268b87b5d09b1091bfd0326ed18ce5b91): rebased
> I think it'd be good to just rebase this and merge it and not try to do the "move the indexes threads start after the loading process" change here.
Ok, I only rebased.
@furszy I like your suggestion and will review/test it when you include them in #27607, which I believe will change init
...
💬 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.