💬 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)
💬 MarcoFalke commented on issue "Script verification being run when rebuilding UTXO database.":
(https://github.com/bitcoin/bitcoin/issues/27639#issuecomment-1545695645)
You can leave `-assumevalid`'s default value to skip the checks. However, if you run into disk corruption, I'd recommend to disable it via `-assumevalid=0` to catch problems in files on disk.
(https://github.com/bitcoin/bitcoin/issues/27639#issuecomment-1545695645)
You can leave `-assumevalid`'s default value to skip the checks. However, if you run into disk corruption, I'd recommend to disable it via `-assumevalid=0` to catch problems in files on disk.
💬 furszy commented on pull request "wallet: when a block is disconnected, update transactions that are no longer conflicted":
(https://github.com/bitcoin/bitcoin/pull/27145#discussion_r1192328933)
good. Not sure if there is any but would be nice to have coverage for the automatic re-submission. Would just need to mock the node time 24hs in the future and see if they get re-submitted.
(https://github.com/bitcoin/bitcoin/pull/27145#discussion_r1192328933)
good. Not sure if there is any but would be nice to have coverage for the automatic re-submission. Would just need to mock the node time 24hs in the future and see if they get re-submitted.
💬 vasild commented on pull request "Raise on invalid -debug and -loglevel config options":
(https://github.com/bitcoin/bitcoin/pull/27632#discussion_r1192332228)
Using `util::Result` here would be self-documenting.
(https://github.com/bitcoin/bitcoin/pull/27632#discussion_r1192332228)
Using `util::Result` here would be self-documenting.
🤔 hebasto reviewed a pull request: "Introduce field element and group element classes to test_framework/key.py"
(https://github.com/bitcoin/bitcoin/pull/26222#pullrequestreview-1424459844)
> To maximize readability...
Concept ACK on that. At least, non-cryptography abstract algebra background is enough to review the code :)
Partially reviewed. The `class FE` looks good.
(https://github.com/bitcoin/bitcoin/pull/26222#pullrequestreview-1424459844)
> To maximize readability...
Concept ACK on that. At least, non-cryptography abstract algebra background is enough to review the code :)
Partially reviewed. The `class FE` looks good.
💬 ryanofsky commented on pull request "p2p: skip netgroup diversity follow-up":
(https://github.com/bitcoin/bitcoin/pull/27467#issuecomment-1545703548)
> > 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
I agree with you that at a project / maintenance level we should be neutral about "I don't think this PR is a good use of effort but I can't point out any actual problems with it" review feedback. If a PR has sufficient r
...
(https://github.com/bitcoin/bitcoin/pull/27467#issuecomment-1545703548)
> > 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
I agree with you that at a project / maintenance level we should be neutral about "I don't think this PR is a good use of effort but I can't point out any actual problems with it" review feedback. If a PR has sufficient r
...
💬 ryanofsky commented on pull request "p2p: skip netgroup diversity follow-up":
(https://github.com/bitcoin/bitcoin/pull/27467#issuecomment-1545712600)
> The code comment needs to be fixed as it is now incorrect, and the naming is now misleading. I think the change is clearly justified here.
Could you say more about how the current code is misleading in the PR description? I'm sure the new code is better, but knowing what is incorrect or misleading in the current code could help motivate the PR a little more
(https://github.com/bitcoin/bitcoin/pull/27467#issuecomment-1545712600)
> The code comment needs to be fixed as it is now incorrect, and the naming is now misleading. I think the change is clearly justified here.
Could you say more about how the current code is misleading in the PR description? I'm sure the new code is better, but knowing what is incorrect or misleading in the current code could help motivate the PR a little more
💬 MarcoFalke commented on pull request "net processing: avoid serving non-announced txs as a result of a MEMPOOL message":
(https://github.com/bitcoin/bitcoin/pull/27602#discussion_r1192361975)
> Even then this approach seems like it can trigger false positives fairly easily, since the rolling bloom filter capacity is only 3500 txs?
I didn't understand this. How is this different from a peer connecting and waiting for 3500 "normal" newly announced transaction to be announced and added to the bloom filter to achieve the same false positive rate?
> or since it's a rolling bloom filter, causing overflow leading to false negatives (leading to functionality bugs where you can't get th
...
(https://github.com/bitcoin/bitcoin/pull/27602#discussion_r1192361975)
> Even then this approach seems like it can trigger false positives fairly easily, since the rolling bloom filter capacity is only 3500 txs?
I didn't understand this. How is this different from a peer connecting and waiting for 3500 "normal" newly announced transaction to be announced and added to the bloom filter to achieve the same false positive rate?
> or since it's a rolling bloom filter, causing overflow leading to false negatives (leading to functionality bugs where you can't get th
...
💬 Sjors commented on pull request "Improve performance of p2p inv to send queues":
(https://github.com/bitcoin/bitcoin/pull/27610#issuecomment-1545737922)
Partial utACK for the changes in `CompareDepthAndScore`. Took me a while to wrap my head around it.
(https://github.com/bitcoin/bitcoin/pull/27610#issuecomment-1545737922)
Partial utACK for the changes in `CompareDepthAndScore`. Took me a while to wrap my head around it.