💬 waketraindev commented on pull request "rpc: add optional peer_ids param to filter getpeerinfo":
(https://github.com/bitcoin/bitcoin/pull/32741#discussion_r2207805466)
I agree, the last push swapped to an unordered_set with an array argument
(https://github.com/bitcoin/bitcoin/pull/32741#discussion_r2207805466)
I agree, the last push swapped to an unordered_set with an array argument
💬 rkrux commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2207804501)
This commented error should be removed now.
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2207804501)
This commented error should be removed now.
🤔 rkrux reviewed a pull request: "wallet, rpc: add v3 transaction creation and wallet support"
(https://github.com/bitcoin/bitcoin/pull/32896#pullrequestreview-3020910288)
Concept ACK b3817a822054fb2ea2964535b5a0ee55dd5d55ac
Thanks for picking this up, I will try to review it soon.
The TODOs seem to be done and can be removed from the PR description.
(https://github.com/bitcoin/bitcoin/pull/32896#pullrequestreview-3020910288)
Concept ACK b3817a822054fb2ea2964535b5a0ee55dd5d55ac
Thanks for picking this up, I will try to review it soon.
The TODOs seem to be done and can be removed from the PR description.
🤔 josibake reviewed a pull request: "Silent Payments: Receiving"
(https://github.com/bitcoin/bitcoin/pull/32966#pullrequestreview-3020599518)
Did a big overview with @Sjors , leaving the notes from our discussion as a review. In general, I think we should investigate using a custom type for the scan key since a lot of the changes seem to be hacking around the fact we represent the scan key as a private key, but then often need to use it as not a private key.
Still in the process of reviewing, but leaving the initial notes for now
(https://github.com/bitcoin/bitcoin/pull/32966#pullrequestreview-3020599518)
Did a big overview with @Sjors , leaving the notes from our discussion as a review. In general, I think we should investigate using a custom type for the scan key since a lot of the changes seem to be hacking around the fact we represent the scan key as a private key, but then often need to use it as not a private key.
Still in the process of reviewing, but leaving the initial notes for now
💬 josibake commented on pull request "Silent Payments: Receiving":
(https://github.com/bitcoin/bitcoin/pull/32966#discussion_r2207630652)
While reviewing the PR, it seems a lot of things could be simplified by if we just treated the scan key as something that _isn't_ a `CKey`, i.e., we introduce a completely new object for holding the scan key. I think that would allow us to drop this commit and simplify other places where we are trying to work around the fact that a scan key isnt really a private key.
I haven't thought this all the way through yet, but in my initial pass on the PR it seems like it should be possible.
(https://github.com/bitcoin/bitcoin/pull/32966#discussion_r2207630652)
While reviewing the PR, it seems a lot of things could be simplified by if we just treated the scan key as something that _isn't_ a `CKey`, i.e., we introduce a completely new object for holding the scan key. I think that would allow us to drop this commit and simplify other places where we are trying to work around the fact that a scan key isnt really a private key.
I haven't thought this all the way through yet, but in my initial pass on the PR it seems like it should be possible.
💬 josibake commented on pull request "Silent Payments: Receiving":
(https://github.com/bitcoin/bitcoin/pull/32966#discussion_r2207667539)
Potentially another area we could simplify if we end up not treating a scan key as a `CKey`.
(https://github.com/bitcoin/bitcoin/pull/32966#discussion_r2207667539)
Potentially another area we could simplify if we end up not treating a scan key as a `CKey`.
💬 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.