💬 ryanofsky commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1953227295)
re: https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937311760
This discussion is interesting but I don't understand the line
https://github.com/bitcoin/bitcoin/blob/8fa10edcd1706a1f0dc9d8c3adbc8efa3c7755bf/src/node/miner.cpp#L157
at all. Why does it even make sense?
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1953227295)
re: https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937311760
This discussion is interesting but I don't understand the line
https://github.com/bitcoin/bitcoin/blob/8fa10edcd1706a1f0dc9d8c3adbc8efa3c7755bf/src/node/miner.cpp#L157
at all. Why does it even make sense?
💬 ryanofsky commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1953213920)
In commit "Add waitNext() to BlockTemplate interface" (7f6f58bad8e56c9a25ffbc0d105bed37e9958fef)
IMO would be slightly better to write this as `options.fee_threshold < MAX_MONEY || tip_changed`, so MAX_MONEY is not a magic value, just an optimization cutoff for the point where we stop checking if fee_threshold has been reached because we know it is too high to be reached.
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1953213920)
In commit "Add waitNext() to BlockTemplate interface" (7f6f58bad8e56c9a25ffbc0d105bed37e9958fef)
IMO would be slightly better to write this as `options.fee_threshold < MAX_MONEY || tip_changed`, so MAX_MONEY is not a magic value, just an optimization cutoff for the point where we stop checking if fee_threshold has been reached because we know it is too high to be reached.
💬 ryanofsky commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1953206134)
In commit "Add waitNext() to BlockTemplate interface" (7f6f58bad8e56c9a25ffbc0d105bed37e9958fef)
IMO it is confusing to define an extra lambda here that is only called in one place (inside another lambda). It would be less confusing to just inline `check_tip_changed` in the place where it is used, to remove a level of indirection and make the order code is written match the order that it executes. Would suggest
<details><summary>diff</summary>
<p>
```diff
--- a/src/node/interfaces.c
...
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1953206134)
In commit "Add waitNext() to BlockTemplate interface" (7f6f58bad8e56c9a25ffbc0d105bed37e9958fef)
IMO it is confusing to define an extra lambda here that is only called in one place (inside another lambda). It would be less confusing to just inline `check_tip_changed` in the place where it is used, to remove a level of indirection and make the order code is written match the order that it executes. Would suggest
<details><summary>diff</summary>
<p>
```diff
--- a/src/node/interfaces.c
...
💬 ryanofsky commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1953247978)
In commit "Add waitNext() to BlockTemplate interface" (7f6f58bad8e56c9a25ffbc0d105bed37e9958fef)
I found this comment pretty confusing and would suggest making it more specific like "// if current_height is odd, block_template will have already been set at the end of the previous loop."
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1953247978)
In commit "Add waitNext() to BlockTemplate interface" (7f6f58bad8e56c9a25ffbc0d105bed37e9958fef)
I found this comment pretty confusing and would suggest making it more specific like "// if current_height is odd, block_template will have already been set at the end of the previous loop."
💬 ryanofsky commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1953275413)
In commit "Add waitNext() to BlockTemplate interface" (7f6f58bad8e56c9a25ffbc0d105bed37e9958fef)
Seem like it would be more correct (and more compact) to write this as:
tip_changed = Assume(tip_block) && tip_block != m_block_template->block.hashPrevBlock;
Since if tip_block is null the right thing to do is just wait for it to be non-null, not set tip_changed to true and try to return a new block template below.
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1953275413)
In commit "Add waitNext() to BlockTemplate interface" (7f6f58bad8e56c9a25ffbc0d105bed37e9958fef)
Seem like it would be more correct (and more compact) to write this as:
tip_changed = Assume(tip_block) && tip_block != m_block_template->block.hashPrevBlock;
Since if tip_block is null the right thing to do is just wait for it to be non-null, not set tip_changed to true and try to return a new block template below.
👍 brunoerg approved a pull request: "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases"
(https://github.com/bitcoin/bitcoin/pull/31495#pullrequestreview-2613027538)
code review ACK af76664b12d8611b606a7e755a103a20542ee539
(https://github.com/bitcoin/bitcoin/pull/31495#pullrequestreview-2613027538)
code review ACK af76664b12d8611b606a7e755a103a20542ee539
💬 darosior commented on pull request "fuzz: add targets for PCP and NAT-PMP port mapping requests":
(https://github.com/bitcoin/bitcoin/pull/31676#issuecomment-2654723569)
Rebased on master. Thanks @marcofleon for fixing the fuzz target for me, i addressed your comments.
(https://github.com/bitcoin/bitcoin/pull/31676#issuecomment-2654723569)
Rebased on master. Thanks @marcofleon for fixing the fuzz target for me, i addressed your comments.
💬 darosior commented on pull request "Doc: add a comment referencing past vulnerability next to where it was fixed":
(https://github.com/bitcoin/bitcoin/pull/30538#issuecomment-2654741147)
Rebased.
(https://github.com/bitcoin/bitcoin/pull/30538#issuecomment-2654741147)
Rebased.
💬 sipa commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1953374431)
I don't feel that's necessary. The calling code is wrong if it's called with an empty `to_remove`. It's hopefully sufficient to document this, so reviewers can be convinced this isn't being violated.
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1953374431)
I don't feel that's necessary. The calling code is wrong if it's called with an empty `to_remove`. It's hopefully sufficient to document this, so reviewers can be convinced this isn't being violated.
💬 jonatack commented on pull request "doc: improve NODE_NETWORK_LIMITED documentation per BIP159":
(https://github.com/bitcoin/bitcoin/pull/31805#discussion_r1953384987)
Done.
(https://github.com/bitcoin/bitcoin/pull/31805#discussion_r1953384987)
Done.
💬 jonatack commented on pull request "doc: improve NODE_NETWORK_LIMITED documentation per BIP159":
(https://github.com/bitcoin/bitcoin/pull/31805#issuecomment-2654830209)
In response to review feedback, I've reduced the scope of this PR and updated the pull description. Originally this tried to clarify the following, but it is now dropped:
`NODE_NETWORK_LIMITED` is set by default, and it only signals a pruned or otherwise limited node if `NODE_NETWORK` is not also set (see the setup logic or `IsLimitedPeer()` in `src/net_processing.cpp` for an example).
(https://github.com/bitcoin/bitcoin/pull/31805#issuecomment-2654830209)
In response to review feedback, I've reduced the scope of this PR and updated the pull description. Originally this tried to clarify the following, but it is now dropped:
`NODE_NETWORK_LIMITED` is set by default, and it only signals a pruned or otherwise limited node if `NODE_NETWORK` is not also set (see the setup logic or `IsLimitedPeer()` in `src/net_processing.cpp` for an example).
💬 l0rinc commented on pull request "Add -pruneduringinit option to temporarily use another prune target during IBD":
(https://github.com/bitcoin/bitcoin/pull/31845#discussion_r1953391405)
Thanks for the details, @andrewtoth.
So basically having a low pruning target during IBD (preferably 0) with high dbcache (preferably max) would be the fastest possible option (especially after https://github.com/bitcoin/bitcoin/pull/31645), right?
@luke-jr, it would be great if we could lower the IBD block writing threshold further (especially since the mentioned 288-block limit already seems incorrect - I'll run an IBD to measure it exactly).
(https://github.com/bitcoin/bitcoin/pull/31845#discussion_r1953391405)
Thanks for the details, @andrewtoth.
So basically having a low pruning target during IBD (preferably 0) with high dbcache (preferably max) would be the fastest possible option (especially after https://github.com/bitcoin/bitcoin/pull/31645), right?
@luke-jr, it would be great if we could lower the IBD block writing threshold further (especially since the mentioned 288-block limit already seems incorrect - I'll run an IBD to measure it exactly).
📝 davidgumberg opened a pull request: "doc: build: Fix instructions for msvc gui builds"
(https://github.com/bitcoin/bitcoin/pull/31851)
If the instructions are followed as-is, and "Developer (PowerShell|Command Prompt) for VS 2022" is used to execute the suggested build commands, the root directory of vcpkg (e.g. in VS 2022 Community edition: `C:\Program Files\Microsoft Visual Studio\2022\Community\VC\vcpkg`), is too long, and when vcpkg attempts to build any of the QT packages, it will fail because of build steps that require path lengths greater than Windows' `MAX_PATH` 260 character limit. This can be avoided without needing
...
(https://github.com/bitcoin/bitcoin/pull/31851)
If the instructions are followed as-is, and "Developer (PowerShell|Command Prompt) for VS 2022" is used to execute the suggested build commands, the root directory of vcpkg (e.g. in VS 2022 Community edition: `C:\Program Files\Microsoft Visual Studio\2022\Community\VC\vcpkg`), is too long, and when vcpkg attempts to build any of the QT packages, it will fail because of build steps that require path lengths greater than Windows' `MAX_PATH` 260 character limit. This can be avoided without needing
...
💬 l0rinc commented on pull request "Add -pruneduringinit option to temporarily use another prune target during IBD":
(https://github.com/bitcoin/bitcoin/pull/31845#discussion_r1953393774)
To see how low we can go safely to speed up IBD
(https://github.com/bitcoin/bitcoin/pull/31845#discussion_r1953393774)
To see how low we can go safely to speed up IBD
💬 mzumsande commented on issue "Stuck in Endless Pre-Syncing Headers Loop":
(https://github.com/bitcoin/bitcoin/issues/26391#issuecomment-2654838081)
> A question is whether we could detect this during the pre-sync phase instead, which wouldn't stop the lack of progress, but would avoid the bandwidth waste on repeated presyncs with everyone. The answer is yes - it wouldn't be hard to check for known-invalid headers in the presync phase as well, however, I don't think we want to do that because of fingerprinting reasons: it would permit an attacker to feed you an invalid, low-PoW, block during IBD, and then later follow you around the network
...
(https://github.com/bitcoin/bitcoin/issues/26391#issuecomment-2654838081)
> A question is whether we could detect this during the pre-sync phase instead, which wouldn't stop the lack of progress, but would avoid the bandwidth waste on repeated presyncs with everyone. The answer is yes - it wouldn't be hard to check for known-invalid headers in the presync phase as well, however, I don't think we want to do that because of fingerprinting reasons: it would permit an attacker to feed you an invalid, low-PoW, block during IBD, and then later follow you around the network
...
💬 sipa commented on issue "Stuck in Endless Pre-Syncing Headers Loop":
(https://github.com/bitcoin/bitcoin/issues/26391#issuecomment-2654843751)
@mzumsande Agreed, reading back what I wrote, I don't see why I thought fingerprinting would be an issue.
I think the most useful course of actual here is detecting the presence of a high-PoW header-invalid chain, and reporting it to the user as a sign of likely corruption. Unsure what that would mean for anything other than the GUI, though.
(https://github.com/bitcoin/bitcoin/issues/26391#issuecomment-2654843751)
@mzumsande Agreed, reading back what I wrote, I don't see why I thought fingerprinting would be an issue.
I think the most useful course of actual here is detecting the presence of a high-PoW header-invalid chain, and reporting it to the user as a sign of likely corruption. Unsure what that would mean for anything other than the GUI, though.
🤔 brunoerg reviewed a pull request: "descriptors: inference process, do not return unparsable multisig descriptors"
(https://github.com/bitcoin/bitcoin/pull/31404#pullrequestreview-2613204326)
It seems `InferDescriptor` can still produce invalid descriptors due to invalid keys. e.g.:
Hex script: `5141046161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161610051ae`
descriptor (invalid): `multi(1,0461616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616100)#jqqp7208`.
(https://github.com/bitcoin/bitcoin/pull/31404#pullrequestreview-2613204326)
It seems `InferDescriptor` can still produce invalid descriptors due to invalid keys. e.g.:
Hex script: `5141046161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161610051ae`
descriptor (invalid): `multi(1,0461616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616100)#jqqp7208`.
💬 ismaelsadeeq commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1953242843)
In "txgraph: (tests) add simulation fuzz test" 1f06bc1e4a8108f1430bcd20fc391c9f663a2e4b
why add 1 and then subtract again when using the choices value?
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1953242843)
In "txgraph: (tests) add simulation fuzz test" 1f06bc1e4a8108f1430bcd20fc391c9f663a2e4b
why add 1 and then subtract again when using the choices value?
💬 ismaelsadeeq commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1953271770)
In "txgraph: (tests) add simulation fuzz test" 1f06bc1e4a8108f1430bcd20fc391c9f663a2e4b
Should we return early when parent ref is the same as ref child? This will prevent a possibility of having a cycle.
As such, we don’t have to check before adding dependency in the fuzz test.
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1953271770)
In "txgraph: (tests) add simulation fuzz test" 1f06bc1e4a8108f1430bcd20fc391c9f663a2e4b
Should we return early when parent ref is the same as ref child? This will prevent a possibility of having a cycle.
As such, we don’t have to check before adding dependency in the fuzz test.
💬 ismaelsadeeq commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1953307646)
In "txgraph: (tests) add simulation fuzz test" 1f06bc1e4a8108f1430bcd20fc391c9f663a2e4b
This loop is quite big, has series of steps that are depending if behavior of the if else branches.
Will be nice to add an overview of how the test works, just like it was done in `cluster_linearize.cpp`
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1953307646)
In "txgraph: (tests) add simulation fuzz test" 1f06bc1e4a8108f1430bcd20fc391c9f663a2e4b
This loop is quite big, has series of steps that are depending if behavior of the if else branches.
Will be nice to add an overview of how the test works, just like it was done in `cluster_linearize.cpp`