💬 josibake commented on pull request "Silent Payments: Receiving":
(https://github.com/bitcoin/bitcoin/pull/32966#discussion_r2207664427)
Would recommend splitting the output type changes into their own commit(s). Its a self-contained change, and splitting them out from the descriptor changes makes the descriptor commit easier to review.
(https://github.com/bitcoin/bitcoin/pull/32966#discussion_r2207664427)
Would recommend splitting the output type changes into their own commit(s). Its a self-contained change, and splitting them out from the descriptor changes makes the descriptor commit easier to review.
💬 josibake commented on pull request "Silent Payments: Receiving":
(https://github.com/bitcoin/bitcoin/pull/32966#discussion_r2207679341)
AFAICT, we only use the `sp_keys` member of the FlatSigningProvider as a way of passing around the scan key and the spend pubkey, and then also for the spend private key (for signing). It makes sense to me that we would later add the spend private key + tweaks to the FlatSigningProvider, but ideally thats the only thing we use it for and can get rid of `sp_keys`.
(https://github.com/bitcoin/bitcoin/pull/32966#discussion_r2207679341)
AFAICT, we only use the `sp_keys` member of the FlatSigningProvider as a way of passing around the scan key and the spend pubkey, and then also for the spend private key (for signing). It makes sense to me that we would later add the spend private key + tweaks to the FlatSigningProvider, but ideally thats the only thing we use it for and can get rid of `sp_keys`.
💬 josibake commented on pull request "Silent Payments: Receiving":
(https://github.com/bitcoin/bitcoin/pull/32966#discussion_r2207696186)
Another place we could simplify things if we instead came up with a custom type for the scan key.
(https://github.com/bitcoin/bitcoin/pull/32966#discussion_r2207696186)
Another place we could simplify things if we instead came up with a custom type for the scan key.
💬 josibake commented on pull request "Silent Payments: Receiving":
(https://github.com/bitcoin/bitcoin/pull/32966#discussion_r2207692663)
nit: unnecessary formatting change, should just be a one-line change.
(https://github.com/bitcoin/bitcoin/pull/32966#discussion_r2207692663)
nit: unnecessary formatting change, should just be a one-line change.
💬 josibake commented on pull request "Silent Payments: Receiving":
(https://github.com/bitcoin/bitcoin/pull/32966#discussion_r2207706515)
This could be moved to the output type commit.
(https://github.com/bitcoin/bitcoin/pull/32966#discussion_r2207706515)
This could be moved to the output type commit.
💬 josibake commented on pull request "Silent Payments: Receiving":
(https://github.com/bitcoin/bitcoin/pull/32966#discussion_r2207717440)
I think its more appropriate to move this commit into the PR where we first introduce `V0SilentPaymentDestination`. I'll cherry pick this out into https://github.com/bitcoin/bitcoin/pull/28122
(https://github.com/bitcoin/bitcoin/pull/32966#discussion_r2207717440)
I think its more appropriate to move this commit into the PR where we first introduce `V0SilentPaymentDestination`. I'll cherry pick this out into https://github.com/bitcoin/bitcoin/pull/28122
💬 josibake commented on pull request "Silent Payments: Receiving":
(https://github.com/bitcoin/bitcoin/pull/32966#discussion_r2207703326)
Feels like the SigningProvider should only care about the `spend_key + tweak` for signing, so another place to potentially simplify things if scan key is a custom type instead of a `CKey`.
(https://github.com/bitcoin/bitcoin/pull/32966#discussion_r2207703326)
Feels like the SigningProvider should only care about the `spend_key + tweak` for signing, so another place to potentially simplify things if scan key is a custom type instead of a `CKey`.
🤔 maflcko reviewed a pull request: "kernel: Move block tree db open to block manager"
(https://github.com/bitcoin/bitcoin/pull/30965#pullrequestreview-3020935890)
sorry for missing the bug in review
(https://github.com/bitcoin/bitcoin/pull/30965#pullrequestreview-3020935890)
sorry for missing the bug in review
💬 maflcko commented on pull request "kernel: Move block tree db open to block manager":
(https://github.com/bitcoin/bitcoin/pull/30965#discussion_r2207822121)
this is my fault for reviewing this without testing, but I don't think you got rid of the "previous loop", the two iterations can still happen and removing this will now deadlock the program.
You'll still have to reset and delete the pointer before constructing the new one.
(https://github.com/bitcoin/bitcoin/pull/30965#discussion_r2207822121)
this is my fault for reviewing this without testing, but I don't think you got rid of the "previous loop", the two iterations can still happen and removing this will now deadlock the program.
You'll still have to reset and delete the pointer before constructing the new one.
💬 Sjors commented on pull request "Silent Payments: Receiving":
(https://github.com/bitcoin/bitcoin/pull/32966#discussion_r2207029910)
cb44e3812c741e3a939a64799130bbf0787ef89a : I think we should set `bool IsRange() const override { return false; }` so we don't have to think about ranged descriptors.
(https://github.com/bitcoin/bitcoin/pull/32966#discussion_r2207029910)
cb44e3812c741e3a939a64799130bbf0787ef89a : I think we should set `bool IsRange() const override { return false; }` so we don't have to think about ranged descriptors.
💬 waketraindev commented on pull request "rpc: add optional peer_ids param to filter getpeerinfo":
(https://github.com/bitcoin/bitcoin/pull/32741#issuecomment-3074170972)
Updated to use `peer_ids` (array) instead of a single `peer_id`. This allows filtering by multiple peers in one call and aligns better with RPC design. Functional tests adjusted.
(https://github.com/bitcoin/bitcoin/pull/32741#issuecomment-3074170972)
Updated to use `peer_ids` (array) instead of a single `peer_id`. This allows filtering by multiple peers in one call and aligns better with RPC design. Functional tests adjusted.
📝 Galoretka opened a pull request: "fix: Python 3 bytes comparison in linearize-data.py"
(https://github.com/bitcoin/bitcoin/pull/32978)
Replace comparison of a bytes element to the string literal "\0" with an integer 0 in linearize-data.py to ensure correct behavior in Python 3. In Python 3, indexing a bytes object returns an integer, not a single-character string, so the previous comparison would always be false. This change improves compatibility and correctness of the script when detecting end-of-file or null bytes.
(https://github.com/bitcoin/bitcoin/pull/32978)
Replace comparison of a bytes element to the string literal "\0" with an integer 0 in linearize-data.py to ensure correct behavior in Python 3. In Python 3, indexing a bytes object returns an integer, not a single-character string, so the previous comparison would always be false. This change improves compatibility and correctness of the script when detecting end-of-file or null bytes.
💬 maflcko commented on pull request "Set notifications m_tip_block in LoadChainTip()":
(https://github.com/bitcoin/bitcoin/pull/31346#discussion_r2207914726)
This is causing a crash in the GUI, when the user selects "Yes" to the question "Do you want to rebuild the databases now?":
```
init.cpp:1831 AppInitMain: Assertion `chainman.ActiveTip()' failed.
```
I presume the reason is that the first notification *before* the reindex here will trick the `kernel_notifications.m_tip_block_cv.wait` to skip immediately, thinking it is already good to continue.
However, it needs to wait for the second notification *after* the reindex.
A possible fix would
...
(https://github.com/bitcoin/bitcoin/pull/31346#discussion_r2207914726)
This is causing a crash in the GUI, when the user selects "Yes" to the question "Do you want to rebuild the databases now?":
```
init.cpp:1831 AppInitMain: Assertion `chainman.ActiveTip()' failed.
```
I presume the reason is that the first notification *before* the reindex here will trick the `kernel_notifications.m_tip_block_cv.wait` to skip immediately, thinking it is already good to continue.
However, it needs to wait for the second notification *after* the reindex.
A possible fix would
...
📝 maflcko opened a pull request: "test: Check that the GUI interactive reindex works"
(https://github.com/bitcoin/bitcoin/pull/32979)
Given that several bugs have been introduced in the last release alone (https://github.com/bitcoin/bitcoin/pull/31346#discussion_r2207914726, https://github.com/bitcoin/bitcoin/pull/30965#discussion_r2207822121, ...), it could make sense to test this code path.
(https://github.com/bitcoin/bitcoin/pull/32979)
Given that several bugs have been introduced in the last release alone (https://github.com/bitcoin/bitcoin/pull/31346#discussion_r2207914726, https://github.com/bitcoin/bitcoin/pull/30965#discussion_r2207822121, ...), it could make sense to test this code path.
💬 instagibbs commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#issuecomment-3074256706)
utACK 2c2acb37ecdeed1582b9835f9075d44e313d787c
Easy to read and appears to comply with the BIP text. Logic seems sound, seems utterly unreasonable to make these kinds of transactions, and in general breaking up these types of transactions will result in transactions working again. This along with not seeing these on-chain for 10 years seems to suggest a minuscule confiscatory surface.
I'm still mulling over the larger accounting changes vs other possible tradeoffs, but this doesn't have to
...
(https://github.com/bitcoin/bitcoin/pull/32521#issuecomment-3074256706)
utACK 2c2acb37ecdeed1582b9835f9075d44e313d787c
Easy to read and appears to comply with the BIP text. Logic seems sound, seems utterly unreasonable to make these kinds of transactions, and in general breaking up these types of transactions will result in transactions working again. This along with not seeing these on-chain for 10 years seems to suggest a minuscule confiscatory surface.
I'm still mulling over the larger accounting changes vs other possible tradeoffs, but this doesn't have to
...
💬 Sjors commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#issuecomment-3074275673)
re-ACK 2c2acb37ecdeed1582b9835f9075d44e313d787c
(https://github.com/bitcoin/bitcoin/pull/32521#issuecomment-3074275673)
re-ACK 2c2acb37ecdeed1582b9835f9075d44e313d787c
🤔 ismaelsadeeq reviewed a pull request: "cluster mempool: add TxGraph work controls"
(https://github.com/bitcoin/bitcoin/pull/32263#pullrequestreview-3020830689)
Code review ACK 62ed1f92efff42bc79c50935e6dbd9da4e072020
Just a question and some nits.
(https://github.com/bitcoin/bitcoin/pull/32263#pullrequestreview-3020830689)
Code review ACK 62ed1f92efff42bc79c50935e6dbd9da4e072020
Just a question and some nits.
💬 ismaelsadeeq commented on pull request "cluster mempool: add TxGraph work controls":
(https://github.com/bitcoin/bitcoin/pull/32263#discussion_r2207756051)
Why size two?
Isn't this supposed to be one? That is, individual splits are already optimal, but any component with size greater than one needs to be linearized, no?
(https://github.com/bitcoin/bitcoin/pull/32263#discussion_r2207756051)
Why size two?
Isn't this supposed to be one? That is, individual splits are already optimal, but any component with size greater than one needs to be linearized, no?
💬 ismaelsadeeq commented on pull request "cluster mempool: add TxGraph work controls":
(https://github.com/bitcoin/bitcoin/pull/32263#discussion_r2207883583)
this is a duplication because `AddDependency` already made `real_is_optimal` to be `false`
(https://github.com/bitcoin/bitcoin/pull/32263#discussion_r2207883583)
this is a duplication because `AddDependency` already made `real_is_optimal` to be `false`
💬 ismaelsadeeq commented on pull request "cluster mempool: add TxGraph work controls":
(https://github.com/bitcoin/bitcoin/pull/32263#discussion_r2207856137)
```suggestion
// Update the Cluster's quality if it has improved.
```
(https://github.com/bitcoin/bitcoin/pull/32263#discussion_r2207856137)
```suggestion
// Update the Cluster's quality if it has improved.
```