💬 andrewtoth commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1472190098)
As noted in https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1454112543, stale tip checks don't factor into the current approach. This check seems redundant now with the one on [lines 54-56](https://github.com/bitcoin/bitcoin/pull/28170/files#diff-aafd7c9690634ecdad15d527c2bab6bffbcddd2db444a6099da0305d2d2271beR54-R56).
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1472190098)
As noted in https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1454112543, stale tip checks don't factor into the current approach. This check seems redundant now with the one on [lines 54-56](https://github.com/bitcoin/bitcoin/pull/28170/files#diff-aafd7c9690634ecdad15d527c2bab6bffbcddd2db444a6099da0305d2d2271beR54-R56).
💬 furszy commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1472203963)
> As noted in #28170 (comment), stale tip checks don't factor into the current approach.
Sure. As it is a nit, will update this comment in the follow-up PR. Thanks!.
> This check seems redundant now with the one on lines 54-56.
I think it's worth checking whether the services flags can still change after receiving blocks from the network. The lines 54-56 are checking updates prior receiving any block.
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1472203963)
> As noted in #28170 (comment), stale tip checks don't factor into the current approach.
Sure. As it is a nit, will update this comment in the follow-up PR. Thanks!.
> This check seems redundant now with the one on lines 54-56.
I think it's worth checking whether the services flags can still change after receiving blocks from the network. The lines 54-56 are checking updates prior receiving any block.
💬 hernanmarino commented on pull request "Debug Console implementation of generate method":
(https://github.com/bitcoin-core/gui/pull/692#issuecomment-1918347305)
Rebased on top of master to fix CI errors.
(https://github.com/bitcoin-core/gui/pull/692#issuecomment-1918347305)
Rebased on top of master to fix CI errors.
🤔 stratospher reviewed a pull request: "test: fix intermittent failure in p2p_v2_earlykeyresponse"
(https://github.com/bitcoin/bitcoin/pull/29352#pullrequestreview-1852942477)
clever find! will ACK after this is fixed.
(https://github.com/bitcoin/bitcoin/pull/29352#pullrequestreview-1852942477)
clever find! will ACK after this is fixed.
💬 stratospher commented on pull request "test: fix intermittent failure in p2p_v2_earlykeyresponse":
(https://github.com/bitcoin/bitcoin/pull/29352#discussion_r1472338409)
1d04d58: i think we should call it a different name - `self.connected` or something like that? i'm still getting a crash with the sleep statement because python is confusing `connection_made` variable and callback function.
(https://github.com/bitcoin/bitcoin/pull/29352#discussion_r1472338409)
1d04d58: i think we should call it a different name - `self.connected` or something like that? i'm still getting a crash with the sleep statement because python is confusing `connection_made` variable and callback function.
🤔 BrandonOdiwuor reviewed a pull request: "debugwindow: update session ID tooltip"
(https://github.com/bitcoin-core/gui/pull/788#pullrequestreview-1853065866)
Concept ACK
(https://github.com/bitcoin-core/gui/pull/788#pullrequestreview-1853065866)
Concept ACK
✅ maflcko closed a pull request: "refactor: deduplicate AmountFromValue() functions"
(https://github.com/bitcoin/bitcoin/pull/28134)
(https://github.com/bitcoin/bitcoin/pull/28134)
🤔 S3RK reviewed a pull request: "Silent Payments: Implement BIP352"
(https://github.com/bitcoin/bitcoin/pull/28122#pullrequestreview-1853145226)
Started reviewing. Basic question first. Which RPCs are supposed to work at this stage? I was under impression, that none RPCs should understand SP addresses yet and the support come with send and receiving PRs. But since you've registered SP destination in `CTXDestination` variant, the SP addresses would be recognised by some RPCs. Is that intended? Should we verify that they return sane results?
(https://github.com/bitcoin/bitcoin/pull/28122#pullrequestreview-1853145226)
Started reviewing. Basic question first. Which RPCs are supposed to work at this stage? I was under impression, that none RPCs should understand SP addresses yet and the support come with send and receiving PRs. But since you've registered SP destination in `CTXDestination` variant, the SP addresses would be recognised by some RPCs. Is that intended? Should we verify that they return sane results?
💬 S3RK commented on pull request "Silent Payments: Implement BIP352":
(https://github.com/bitcoin/bitcoin/pull/28122#discussion_r1472465541)
Not sure protocol implementation needs to now about the position in tx.vout array. AFAIK, output calculation doesn't depend on the position. Have you considered passing destinations one by one together with the counter if a destination is repeated?
(https://github.com/bitcoin/bitcoin/pull/28122#discussion_r1472465541)
Not sure protocol implementation needs to now about the position in tx.vout array. AFAIK, output calculation doesn't depend on the position. Have you considered passing destinations one by one together with the counter if a destination is repeated?
💬 S3RK commented on pull request "Silent Payments: Implement BIP352":
(https://github.com/bitcoin/bitcoin/pull/28122#discussion_r1472466841)
I'm too dumb to understand why do we need to return sum on input pubkeys together with the input hash. Does it have something to do with index and light clients?
(https://github.com/bitcoin/bitcoin/pull/28122#discussion_r1472466841)
I'm too dumb to understand why do we need to return sum on input pubkeys together with the input hash. Does it have something to do with index and light clients?
👍 maflcko approved a pull request: "test: fix wallet_import_rescan unrounded minimum amount"
(https://github.com/bitcoin/bitcoin/pull/29343#pullrequestreview-1853179128)
lgtm
(https://github.com/bitcoin/bitcoin/pull/29343#pullrequestreview-1853179128)
lgtm
💬 maflcko commented on pull request "Stratum v2 Noise Protocol":
(https://github.com/bitcoin/bitcoin/pull/29346#discussion_r1472497733)
You can't serialize a non-fixed-size vector as a span. This will lead to errors when deserializing. For example, a default constructed empty vector, serialized, can not be serialized into a vector which has been resized to 64 bytes.
(https://github.com/bitcoin/bitcoin/pull/29346#discussion_r1472497733)
You can't serialize a non-fixed-size vector as a span. This will lead to errors when deserializing. For example, a default constructed empty vector, serialized, can not be serialized into a vector which has been resized to 64 bytes.
💬 glozow commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1472578055)
Yes I'd expect so
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1472578055)
Yes I'd expect so
🚀 fanquake merged a pull request: "test: fix wallet_import_rescan unrounded minimum amount"
(https://github.com/bitcoin/bitcoin/pull/29343)
(https://github.com/bitcoin/bitcoin/pull/29343)
💬 glozow commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1472586945)
Ah makes sense! I think it would be good to add a test for this case?
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1472586945)
Ah makes sense! I think it would be good to add a test for this case?
💬 maflcko commented on pull request "assumeutxo, rpc: Add 'start' parameter to loadtxoutset":
(https://github.com/bitcoin/bitcoin/pull/28659#issuecomment-1918815388)
Ok, maybe turn it into draft, until you are done and the CI is passing?
(https://github.com/bitcoin/bitcoin/pull/28659#issuecomment-1918815388)
Ok, maybe turn it into draft, until you are done and the CI is passing?
🤔 fanquake reviewed a pull request: "init: settings, do not load auto-generated warning msg"
(https://github.com/bitcoin/bitcoin/pull/29301#pullrequestreview-1853403738)
Seems fine. Gets rid of the pointless `2024-01-31T10:25:34Z Ignoring unknown rw_settings value _warning_` and
`2024-01-31T10:25:34Z Setting file arg: _warning_ = "This file is automatically generated and updated by Bitcoin Core. Please do not edit this file while the node is running, as any changes might be ignored or overwritten."` output.
(https://github.com/bitcoin/bitcoin/pull/29301#pullrequestreview-1853403738)
Seems fine. Gets rid of the pointless `2024-01-31T10:25:34Z Ignoring unknown rw_settings value _warning_` and
`2024-01-31T10:25:34Z Setting file arg: _warning_ = "This file is automatically generated and updated by Bitcoin Core. Please do not edit this file while the node is running, as any changes might be ignored or overwritten."` output.
💬 glozow commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1472632779)
yeah not sigops adjusted, but we don't expect that to be necessary here so I think it's fine with a comment
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1472632779)
yeah not sigops adjusted, but we don't expect that to be necessary here so I think it's fine with a comment
💬 josibake commented on pull request "Silent Payments: Implement BIP352":
(https://github.com/bitcoin/bitcoin/pull/28122#discussion_r1472650551)
Yep! There are two ways to use this data:
1. Immediately, as `(b_scan * input_hash) * sum_input_pubkeys`. This is one scalar multiplication and one EC multiplication.
2. Store it for later (either for wallet re-scans or to serve to light clients), as `input_hash * sum_input_pubkeys`. This is one EC multiplication and allows us to store all of the input data needed as 33 bytes.
If the function instead returned `input_hash * sum_input_pubkeys`, we would end up doing two EC multiplications f
...
(https://github.com/bitcoin/bitcoin/pull/28122#discussion_r1472650551)
Yep! There are two ways to use this data:
1. Immediately, as `(b_scan * input_hash) * sum_input_pubkeys`. This is one scalar multiplication and one EC multiplication.
2. Store it for later (either for wallet re-scans or to serve to light clients), as `input_hash * sum_input_pubkeys`. This is one EC multiplication and allows us to store all of the input data needed as 33 bytes.
If the function instead returned `input_hash * sum_input_pubkeys`, we would end up doing two EC multiplications f
...
💬 glozow commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1472680361)
I think indeed we cannot hit this, but it's because we call `ApplyV3Rules` in a package.
When `package` is a parent+child, `has_mempool_sibling` doesn't matter since having any mempool ancestor should cause this to fail immediately. In that way, this function kind of assumes that `package` is a connected component.
If `package` has unrelated transactions, we'd let a mempool sibling slip through in this function. However, since we are calling `ApplyV3Rules` within `PreChecks`, the mempool s
...
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1472680361)
I think indeed we cannot hit this, but it's because we call `ApplyV3Rules` in a package.
When `package` is a parent+child, `has_mempool_sibling` doesn't matter since having any mempool ancestor should cause this to fail immediately. In that way, this function kind of assumes that `package` is a connected component.
If `package` has unrelated transactions, we'd let a mempool sibling slip through in this function. However, since we are calling `ApplyV3Rules` within `PreChecks`, the mempool s
...