💬 dergoegge commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1192138027)
This could be a `else if` of the outer if statement. That would avoid the nesting here.
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1192138027)
This could be a `else if` of the outer if statement. That would avoid the nesting here.
💬 vasild commented on pull request "p2p: skip netgroup diversity follow-up":
(https://github.com/bitcoin/bitcoin/pull/27467#issuecomment-1545496847)
> I don't think it's useful to have these kind of PRs
Ok, this is your opinion. Sometimes I feel the same for some PRs. My reaction is then to ignore them - don't spend _my_ time on them if _I_ think it is not worth it. But I don't try to impose my opinion on others - if somebody was thinking it is worth to open a PR and somebody else was thinking it is worth to review, then let it be. Those other people have different opinion than mine and I am not going to tell them what to work on or what
...
(https://github.com/bitcoin/bitcoin/pull/27467#issuecomment-1545496847)
> I don't think it's useful to have these kind of PRs
Ok, this is your opinion. Sometimes I feel the same for some PRs. My reaction is then to ignore them - don't spend _my_ time on them if _I_ think it is not worth it. But I don't try to impose my opinion on others - if somebody was thinking it is worth to open a PR and somebody else was thinking it is worth to review, then let it be. Those other people have different opinion than mine and I am not going to tell them what to work on or what
...
💬 kroese commented on issue "CPU DoS on mainnet in debug mode":
(https://github.com/bitcoin/bitcoin/issues/27586#issuecomment-1545510083)
> You probably need to restart the node to clear it up after it's started spinning
You were right.. restarting it dropped CPU usage from 100 percent to about 3 percent immediately. I hope it will stay this way as rc3 (which should fix it) still hasn't been released.
(https://github.com/bitcoin/bitcoin/issues/27586#issuecomment-1545510083)
> You probably need to restart the node to clear it up after it's started spinning
You were right.. restarting it dropped CPU usage from 100 percent to about 3 percent immediately. I hope it will stay this way as rc3 (which should fix it) still hasn't been released.
👍 vasild approved a pull request: "p2p: cleanup `LookupIntern`, `Lookup` and `LookupHost`"
(https://github.com/bitcoin/bitcoin/pull/26261#pullrequestreview-1424217567)
ACK 4eee95e57bb6f773bcaeb405bca949f158a62134
(https://github.com/bitcoin/bitcoin/pull/26261#pullrequestreview-1424217567)
ACK 4eee95e57bb6f773bcaeb405bca949f158a62134
💬 fanquake commented on pull request "build: Fix shared lib linking for darwin with lld":
(https://github.com/bitcoin/bitcoin/pull/27628#issuecomment-1545559220)
Guix Build:
```bash
0e6772d00e899aea446bbda37b198142812b95830e4c8f52bde9992efd8bc570 guix-build-d576d69e139b/output/aarch64-linux-gnu/SHA256SUMS.part
f0b4ac500389b012f78c20043adef975e2a33b8e079bbee07b3e4c130c4f67a9 guix-build-d576d69e139b/output/aarch64-linux-gnu/bitcoin-d576d69e139b-aarch64-linux-gnu-debug.tar.gz
51e611952c543a7ad9933934c6b2160703ae2ebe36804b30446a874e49f85130 guix-build-d576d69e139b/output/aarch64-linux-gnu/bitcoin-d576d69e139b-aarch64-linux-gnu.tar.gz
feab71eebaf317b8
...
(https://github.com/bitcoin/bitcoin/pull/27628#issuecomment-1545559220)
Guix Build:
```bash
0e6772d00e899aea446bbda37b198142812b95830e4c8f52bde9992efd8bc570 guix-build-d576d69e139b/output/aarch64-linux-gnu/SHA256SUMS.part
f0b4ac500389b012f78c20043adef975e2a33b8e079bbee07b3e4c130c4f67a9 guix-build-d576d69e139b/output/aarch64-linux-gnu/bitcoin-d576d69e139b-aarch64-linux-gnu-debug.tar.gz
51e611952c543a7ad9933934c6b2160703ae2ebe36804b30446a874e49f85130 guix-build-d576d69e139b/output/aarch64-linux-gnu/bitcoin-d576d69e139b-aarch64-linux-gnu.tar.gz
feab71eebaf317b8
...
💬 rebroad commented on issue "assumeutxo":
(https://github.com/bitcoin/bitcoin/issues/15605#issuecomment-1545571482)
Regarding bootstrapping a pruned node from a full-node, the way I'd do it is to bitcoin-cli the running node to stop it accepting any more blocks or transactions, copy --reflink=always the block index and chainstate to a new bitcoind instance, bitcoin-cli the running node to resume accepting blocks and transactions, modify the new instance to use new ports, and enable pruning, and run it, wait for it to prune the block index, and then exit, and voila, I have a ready to copy bitcoind instance tha
...
(https://github.com/bitcoin/bitcoin/issues/15605#issuecomment-1545571482)
Regarding bootstrapping a pruned node from a full-node, the way I'd do it is to bitcoin-cli the running node to stop it accepting any more blocks or transactions, copy --reflink=always the block index and chainstate to a new bitcoind instance, bitcoin-cli the running node to resume accepting blocks and transactions, modify the new instance to use new ports, and enable pruning, and run it, wait for it to prune the block index, and then exit, and voila, I have a ready to copy bitcoind instance tha
...
💬 vasild commented on pull request "test: add end-to-end tests for CConnman and PeerManager":
(https://github.com/bitcoin/bitcoin/pull/26812#issuecomment-1545603904)
`a81fe4ff9b...f559067e27`: rebase due to conflicts, thanks for looking into this!
(https://github.com/bitcoin/bitcoin/pull/26812#issuecomment-1545603904)
`a81fe4ff9b...f559067e27`: rebase due to conflicts, thanks for looking into this!
⚠️ rebroad opened an issue: "Script verification being run when rebuilding UTXO database."
(https://github.com/bitcoin/bitcoin/issues/27639)
### Please describe the feature you'd like to see added.
It seems that script verification is being run during the rebuilding of the UTXO database, which, if the intention is simply to rebuild the UTXO (due to a disk corruption) then this ought to be unnecessary given the verification has already occurred.
GPT-4 suggested a simple code-change, although it's a little more complex than this as it needs to ensure that the script verification is only skipped on blocks we can be sure have previou
...
(https://github.com/bitcoin/bitcoin/issues/27639)
### Please describe the feature you'd like to see added.
It seems that script verification is being run during the rebuilding of the UTXO database, which, if the intention is simply to rebuild the UTXO (due to a disk corruption) then this ought to be unnecessary given the verification has already occurred.
GPT-4 suggested a simple code-change, although it's a little more complex than this as it needs to ensure that the script verification is only skipped on blocks we can be sure have previou
...
💬 dergoegge commented on pull request "p2p: Increase tx relay rate":
(https://github.com/bitcoin/bitcoin/pull/27630#issuecomment-1545662869)
Concept ACK - 7 tx/s seems outdated
It would be nice if we had a proper simulation framework for stuff like this. For example, what would happen during a mempool storm like we had this past week, if only a portion (e.g. 50%) of the network upgrades to this patch? Would that increase the load on the nodes that are still clearing their relay queues at only 7 tx/s (ignoring #27610 here)?
(https://github.com/bitcoin/bitcoin/pull/27630#issuecomment-1545662869)
Concept ACK - 7 tx/s seems outdated
It would be nice if we had a proper simulation framework for stuff like this. For example, what would happen during a mempool storm like we had this past week, if only a portion (e.g. 50%) of the network upgrades to this patch? Would that increase the load on the nodes that are still clearing their relay queues at only 7 tx/s (ignoring #27610 here)?
👍 ryanofsky approved a pull request: "kernel: Remove interface_ui, util/system from kernel library"
(https://github.com/bitcoin/bitcoin/pull/27636#pullrequestreview-1424328154)
Approach ACK. I think adding the level of indirection here is right approach, but I left some suggestions below which I think would make the implementation less awkward and more future-proof
(https://github.com/bitcoin/bitcoin/pull/27636#pullrequestreview-1424328154)
Approach ACK. I think adding the level of indirection here is right approach, but I left some suggestions below which I think would make the implementation less awkward and more future-proof
💬 ryanofsky commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1192290165)
These new AbortNode calls seem more awkward. And they aren't really future-proof since `AbortNode` calls the global `StartShutdown` function which is specific to the node and will have to be changed for the kernel. Also the name "abort node" doesn't make sense in the kernel context if the kernel application isn't a bitcoin node. Would suggest adding a `FatalError` notification to complement `InitError` in `ChainstateNotifications` class suggested previously:
```c++
class ChainstateNotificati
...
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1192290165)
These new AbortNode calls seem more awkward. And they aren't really future-proof since `AbortNode` calls the global `StartShutdown` function which is specific to the node and will have to be changed for the kernel. Also the name "abort node" doesn't make sense in the kernel context if the kernel application isn't a bitcoin node. Would suggest adding a `FatalError` notification to complement `InitError` in `ChainstateNotifications` class suggested previously:
```c++
class ChainstateNotificati
...
💬 ryanofsky commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1192264695)
It seems like it would be more natural to write this as:
```c++
class ChainstateNotifications
{
virtual void NotifyBlockTip(SynchronizationState state, CBlockIndex* index) {}
virtual void NotifyHeaderTip(SynchronizationState state, int64_t height, int64_t timestamp, bool presync) {}
virtual void ShowProgress(const std::string& title, int nProgress, bool resume_possible) {}
virtual void DoWarning(const bilingual_str& warning) {}
virtual void InitError(const bilingual
...
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1192264695)
It seems like it would be more natural to write this as:
```c++
class ChainstateNotifications
{
virtual void NotifyBlockTip(SynchronizationState state, CBlockIndex* index) {}
virtual void NotifyHeaderTip(SynchronizationState state, int64_t height, int64_t timestamp, bool presync) {}
virtual void ShowProgress(const std::string& title, int nProgress, bool resume_possible) {}
virtual void DoWarning(const bilingual_str& warning) {}
virtual void InitError(const bilingual
...
💬 ryanofsky commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1192295222)
I don't think I would tie these notifications to the ChainStateManager class specifically as they really make sense anywhere in validation code. Would suggest changing `chainstatemanager_notifications` to `chainstate_notifications`, or `validation_notifications` and dropping the "manager" in these places
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1192295222)
I don't think I would tie these notifications to the ChainStateManager class specifically as they really make sense anywhere in validation code. Would suggest changing `chainstatemanager_notifications` to `chainstate_notifications`, or `validation_notifications` and dropping the "manager" in these places
💬 vasild commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1192301612)
Yes, and maybe also send `wtxidrelay`. Any reason not to send `TX` directly and avoid the `INV`, like below?
* VERSION
* [after receiving VERSION] WTXIDRELAY VERACK
* [after receiving VERACK] TX
* PING
* [after receiving PONG] disconnect
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1192301612)
Yes, and maybe also send `wtxidrelay`. Any reason not to send `TX` directly and avoid the `INV`, like below?
* VERSION
* [after receiving VERSION] WTXIDRELAY VERACK
* [after receiving VERACK] TX
* PING
* [after receiving PONG] disconnect
👍 fanquake approved a pull request: "build: Fix shared lib linking for darwin with lld"
(https://github.com/bitcoin/bitcoin/pull/27628#pullrequestreview-1424424089)
ACK d576d69e139ba5724c87d405d6161dc062ddc6f7 - also tested via 21778
(https://github.com/bitcoin/bitcoin/pull/27628#pullrequestreview-1424424089)
ACK d576d69e139ba5724c87d405d6161dc062ddc6f7 - also tested via 21778
💬 fanquake commented on pull request "build: Include qt sources for parsing with extract_strings.py":
(https://github.com/bitcoin/bitcoin/pull/22764#issuecomment-1545675599)
@jarolrod want to followup here?
(https://github.com/bitcoin/bitcoin/pull/22764#issuecomment-1545675599)
@jarolrod want to followup here?
💬 ryanofsky commented on pull request "Raise on invalid -debug and -loglevel config options":
(https://github.com/bitcoin/bitcoin/pull/27632#discussion_r1192306671)
> nit: document those as the return value may not be so obvious to everybody: "if a non-empty optional is returned then an error occurred and it contains the error message".
It's good to document, but this convention does seem to be increasingly used. I have a PR to replace `std::optional<bilingual_str>` with `util::Result` various places (74655e5870888e0165c62fec75000fe04f062147, #25977)
(https://github.com/bitcoin/bitcoin/pull/27632#discussion_r1192306671)
> nit: document those as the return value may not be so obvious to everybody: "if a non-empty optional is returned then an error occurred and it contains the error message".
It's good to document, but this convention does seem to be increasingly used. I have a PR to replace `std::optional<bilingual_str>` with `util::Result` various places (74655e5870888e0165c62fec75000fe04f062147, #25977)
💬 vasild commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#issuecomment-1545683197)
Thanks all for the feedback!
> Just to clarify: We would open the short-lived connection to relay our own transaction and close it right after and then, we wait until we receive an INV about that tx from somebody, if not, we would try to do it (open a new short-lived conn...) all over again?
Yes. But we may as well send to a few peers at the same time, then wait. And, I am not sure, maybe if it does not work for a long time, then fall back to the existent mechanism?
The worst that can h
...
(https://github.com/bitcoin/bitcoin/pull/27509#issuecomment-1545683197)
Thanks all for the feedback!
> Just to clarify: We would open the short-lived connection to relay our own transaction and close it right after and then, we wait until we receive an INV about that tx from somebody, if not, we would try to do it (open a new short-lived conn...) all over again?
Yes. But we may as well send to a few peers at the same time, then wait. And, I am not sure, maybe if it does not work for a long time, then fall back to the existent mechanism?
The worst that can h
...
💬 furszy commented on pull request "wallet: fix deadlock in bdb read write operation":
(https://github.com/bitcoin/bitcoin/pull/27556#issuecomment-1545695358)
> Does this PR makes
>
> https://github.com/bitcoin/bitcoin/blob/137a98c5a22e058ed7a7997a0a4dbd75301de51e/test/sanitizer_suppressions/tsan#L28
>
> outdated?
@hebasto hmm, interesting. It seems to be outdated in master. Just ran the test suite, without the suppression, in master and it passed.
(https://github.com/bitcoin/bitcoin/pull/27556#issuecomment-1545695358)
> Does this PR makes
>
> https://github.com/bitcoin/bitcoin/blob/137a98c5a22e058ed7a7997a0a4dbd75301de51e/test/sanitizer_suppressions/tsan#L28
>
> outdated?
@hebasto hmm, interesting. It seems to be outdated in master. Just ran the test suite, without the suppression, in master and it passed.
✅ MarcoFalke closed an issue: "Script verification being run when rebuilding UTXO database."
(https://github.com/bitcoin/bitcoin/issues/27639)
(https://github.com/bitcoin/bitcoin/issues/27639)