💬 hebasto commented on pull request "ci: A few fixes of `ccache` issues":
(https://github.com/bitcoin/bitcoin/pull/27084#issuecomment-1551531499)
> What is the status of this
I've analyzed the [recent](https://cirrus-ci.com/build/5086582458286080) `ccache` summaries at https://github.com/bitcoin/bitcoin/commit/a75c77ea903c100531e0fc5fde94bb9b52642145.
I don't think this PR can improve them.
(https://github.com/bitcoin/bitcoin/pull/27084#issuecomment-1551531499)
> What is the status of this
I've analyzed the [recent](https://cirrus-ci.com/build/5086582458286080) `ccache` summaries at https://github.com/bitcoin/bitcoin/commit/a75c77ea903c100531e0fc5fde94bb9b52642145.
I don't think this PR can improve them.
✅ hebasto closed a pull request: "ci: A few fixes of `ccache` issues"
(https://github.com/bitcoin/bitcoin/pull/27084)
(https://github.com/bitcoin/bitcoin/pull/27084)
💬 fjahr commented on pull request "assumeutxo: net_processing changes":
(https://github.com/bitcoin/bitcoin/pull/24008#issuecomment-1551540184)
> I guess a simpler way to phrase it is: what can go wrong if [c14ae13](https://github.com/bitcoin/bitcoin/commit/c14ae132c5a5204a9a755c84c6de05fb30459221) picks the incorrect chainstate for a block? What are all the different ways in which we can receive a block? What do we expect to happen in each of these cases? Tests would be a really nice way to illustrate that, but are apparently a pain to write for net_processing.
@Sjors I have tried to create an overview of the different cases and add
...
(https://github.com/bitcoin/bitcoin/pull/24008#issuecomment-1551540184)
> I guess a simpler way to phrase it is: what can go wrong if [c14ae13](https://github.com/bitcoin/bitcoin/commit/c14ae132c5a5204a9a755c84c6de05fb30459221) picks the incorrect chainstate for a block? What are all the different ways in which we can receive a block? What do we expect to happen in each of these cases? Tests would be a really nice way to illustrate that, but are apparently a pain to write for net_processing.
@Sjors I have tried to create an overview of the different cases and add
...
👍 ryanofsky approved a pull request: "kernel: Remove interface_ui, util/system from kernel library"
(https://github.com/bitcoin/bitcoin/pull/27636#pullrequestreview-1430775316)
Code review ACK 0b88c307a8ed81705cf8e6fb6332fdf969eb0e2e. These changes all seem very clean and straightforward now. I left some more suggestions, but you can feel free to ignore them them
(https://github.com/bitcoin/bitcoin/pull/27636#pullrequestreview-1430775316)
Code review ACK 0b88c307a8ed81705cf8e6fb6332fdf969eb0e2e. These changes all seem very clean and straightforward now. I left some more suggestions, but you can feel free to ignore them them
💬 ryanofsky commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1196631303)
In commit "kernel: Add fatalError method to notifications" (0b88c307a8ed81705cf8e6fb6332fdf969eb0e2e)
Would maybe make `notifications` the first parameter. It seems awkward for it to placed between the debug message and the user message.
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1196631303)
In commit "kernel: Add fatalError method to notifications" (0b88c307a8ed81705cf8e6fb6332fdf969eb0e2e)
Would maybe make `notifications` the first parameter. It seems awkward for it to placed between the debug message and the user message.
💬 ryanofsky commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1196623448)
In commit "refactor: Move system from util to common library" (0a7b3e5058b58c448bbc473ffb347910d781c3bd)
I think it makes sense to keep `util::insert` and `util::AnyPtr` functions in `util/`, not `common/` as these are header-only template functions with no dependencies, and there's no particular reason they shouldn't be used by kernel code even if they aren't used right now. In terms of functionality they are also more related to other helper functions like `util/overloaded.h` `util/strings.
...
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1196623448)
In commit "refactor: Move system from util to common library" (0a7b3e5058b58c448bbc473ffb347910d781c3bd)
I think it makes sense to keep `util::insert` and `util::AnyPtr` functions in `util/`, not `common/` as these are header-only template functions with no dependencies, and there's no particular reason they shouldn't be used by kernel code even if they aren't used right now. In terms of functionality they are also more related to other helper functions like `util/overloaded.h` `util/strings.
...
💬 ryanofsky commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1196568945)
In commit "kernel: Add notification interface" (c6f2fbef2c8ecc0ded3abb9ab2fdd5369e954d52)
It would be good to write this accessor as an inline method `kernel::Notifications& GetNotifications() const { return m_options.notifications; }` for discoverability and consistency with other accessors above. Could also drop "Get" prefix since the other accessors don't seem to have it, but whatever you prefer is fine.
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1196568945)
In commit "kernel: Add notification interface" (c6f2fbef2c8ecc0ded3abb9ab2fdd5369e954d52)
It would be good to write this accessor as an inline method `kernel::Notifications& GetNotifications() const { return m_options.notifications; }` for discoverability and consistency with other accessors above. Could also drop "Get" prefix since the other accessors don't seem to have it, but whatever you prefer is fine.
💬 ryanofsky commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1196627541)
In commit "kernel: Add fatalError method to notifications" (0b88c307a8ed81705cf8e6fb6332fdf969eb0e2e)
Would be good to change `bilingual_str` to `const bilingual_str&`. That should also avoid the need to remove the forward declaration and include `#include <util/translation.h>` above.
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1196627541)
In commit "kernel: Add fatalError method to notifications" (0b88c307a8ed81705cf8e6fb6332fdf969eb0e2e)
Would be good to change `bilingual_str` to `const bilingual_str&`. That should also avoid the need to remove the forward declaration and include `#include <util/translation.h>` above.
💬 MarcoFalke commented on pull request "ci: Use credits for ARM task":
(https://github.com/bitcoin/bitcoin/pull/27690#issuecomment-1551551092)
added a commit to improve tsan scheduling times as well
(https://github.com/bitcoin/bitcoin/pull/27690#issuecomment-1551551092)
added a commit to improve tsan scheduling times as well
💬 fanquake commented on pull request "depends: Boost 1.82.0":
(https://github.com/bitcoin/bitcoin/pull/27671#issuecomment-1551562113)
I'll leave this as part of #24742 for now.
(https://github.com/bitcoin/bitcoin/pull/27671#issuecomment-1551562113)
I'll leave this as part of #24742 for now.
✅ fanquake closed a pull request: "depends: Boost 1.82.0"
(https://github.com/bitcoin/bitcoin/pull/27671)
(https://github.com/bitcoin/bitcoin/pull/27671)
✅ fanquake closed a pull request: "p2p: remove adjusted time"
(https://github.com/bitcoin/bitcoin/pull/25908)
(https://github.com/bitcoin/bitcoin/pull/25908)
💬 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.