👍 TheCharlatan approved a pull request: "Add functional test for IPC interface"
(https://github.com/bitcoin/bitcoin/pull/33201#pullrequestreview-3189590307)
ACK a341e11ac92b30aac856049c696a9ac39854569d
(https://github.com/bitcoin/bitcoin/pull/33201#pullrequestreview-3189590307)
ACK a341e11ac92b30aac856049c696a9ac39854569d
💬 Crypt-iQ commented on pull request "net: check for empty header before calling FillBlock":
(https://github.com/bitcoin/bitcoin/pull/33296#discussion_r2325267840)
Yup, like @mzumsande pointed out, this shouldn't be a change in behavior since they would previously hit the `READ_STATUS_INVALID` case.
I didn't consider noban/manual connections for stalling block download as it seems like this is already possible by triggering either of the `READ_STATUS_INVALID` cases (whether [here](https://github.com/bitcoin/bitcoin/blob/37c21ebe4078d5a7b9d8a91aca2ab8b118b9d69f/src/net_processing.cpp#L4432) or [here](https://github.com/bitcoin/bitcoin/blob/37c21ebe4078d5
...
(https://github.com/bitcoin/bitcoin/pull/33296#discussion_r2325267840)
Yup, like @mzumsande pointed out, this shouldn't be a change in behavior since they would previously hit the `READ_STATUS_INVALID` case.
I didn't consider noban/manual connections for stalling block download as it seems like this is already possible by triggering either of the `READ_STATUS_INVALID` cases (whether [here](https://github.com/bitcoin/bitcoin/blob/37c21ebe4078d5a7b9d8a91aca2ab8b118b9d69f/src/net_processing.cpp#L4432) or [here](https://github.com/bitcoin/bitcoin/blob/37c21ebe4078d5
...
💬 hebasto commented on pull request "ci: Checkout latest merged pulls":
(https://github.com/bitcoin/bitcoin/pull/33303#discussion_r2325276845)
The helpful explanatory comment was removed in the recent push (faeb3320952906b6147b06170059e71d7d59f4bd).
Could it be re-added, if it’s not too much trouble?
(https://github.com/bitcoin/bitcoin/pull/33303#discussion_r2325276845)
The helpful explanatory comment was removed in the recent push (faeb3320952906b6147b06170059e71d7d59f4bd).
Could it be re-added, if it’s not too much trouble?
💬 l0rinc commented on pull request "test/refactor: use test deque to avoid quadratic iteration":
(https://github.com/bitcoin/bitcoin/pull/33313#discussion_r2325286088)
Yeah, but two lines below we're already using +=
(https://github.com/bitcoin/bitcoin/pull/33313#discussion_r2325286088)
Yeah, but two lines below we're already using +=
💬 Crypt-iQ commented on pull request "net: check for empty header before calling FillBlock":
(https://github.com/bitcoin/bitcoin/pull/33296#discussion_r2325294058)
Thanks, this is less verbose. I've given commit attribution since I copied the text verbatim.
(https://github.com/bitcoin/bitcoin/pull/33296#discussion_r2325294058)
Thanks, this is less verbose. I've given commit attribution since I copied the text verbatim.
💬 instagibbs commented on pull request "net: check for empty header before calling FillBlock":
(https://github.com/bitcoin/bitcoin/pull/33296#discussion_r2325297443)
thanks, as a future reader I always get suspicious the longer the comment is :)
(https://github.com/bitcoin/bitcoin/pull/33296#discussion_r2325297443)
thanks, as a future reader I always get suspicious the longer the comment is :)
💬 andrewtoth commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-3258611648)
I tried sending a tx with fee rate 0.1 sat/vbyte using 4327327dd07457621fe1a9c88dfbf6fccb0e8854.
I connected to 3 nodes and disconnected after receiving the PONG, and then attempted one more connection every 10 minutes.
I finally received the tx back from another peer after 13 hours and 90 connections.
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-3258611648)
I tried sending a tx with fee rate 0.1 sat/vbyte using 4327327dd07457621fe1a9c88dfbf6fccb0e8854.
I connected to 3 nodes and disconnected after receiving the PONG, and then attempted one more connection every 10 minutes.
I finally received the tx back from another peer after 13 hours and 90 connections.
💬 josibake commented on pull request "Silent Payments: Receiving":
(https://github.com/bitcoin/bitcoin/pull/32966#discussion_r2325246885)
You mention it in the commit message, but I think its worth adding a detailed comment above that:
* Explains the nuance around `m_scan_key`
* Explains why we are using a `PubKeyProvider`, e.g., getting origin information without needing to write custom code
I think a comment is slightly better than a commit message as I imagine this will trip people up in the future who are looking at this code and surprised to see a private key as a member field inside a pubkey provider.
(https://github.com/bitcoin/bitcoin/pull/32966#discussion_r2325246885)
You mention it in the commit message, but I think its worth adding a detailed comment above that:
* Explains the nuance around `m_scan_key`
* Explains why we are using a `PubKeyProvider`, e.g., getting origin information without needing to write custom code
I think a comment is slightly better than a commit message as I imagine this will trip people up in the future who are looking at this code and surprised to see a private key as a member field inside a pubkey provider.
💬 josibake commented on pull request "Silent Payments: Receiving":
(https://github.com/bitcoin/bitcoin/pull/32966#discussion_r2325253065)
In `ToPrivateString` above , you omit the argument name to indicate its not used; should do the same here:
```suggestion
void GetPrivKey(int, const SigningProvider&, FlatSigningProvider& out) const override
```
(https://github.com/bitcoin/bitcoin/pull/32966#discussion_r2325253065)
In `ToPrivateString` above , you omit the argument name to indicate its not used; should do the same here:
```suggestion
void GetPrivKey(int, const SigningProvider&, FlatSigningProvider& out) const override
```
💬 josibake commented on pull request "Silent Payments: Receiving":
(https://github.com/bitcoin/bitcoin/pull/32966#discussion_r2325279070)
I think we could also remove this function by introducing a `SilentPaymentsSign` function (similar to how we have a taproot sign function). I'll dig into this a bit more, but it seems better to have a special signing function that can _only_ be used for silent payments, instead of a generic `CKey::TweakAdd` function that might be abused/footgunny.
(https://github.com/bitcoin/bitcoin/pull/32966#discussion_r2325279070)
I think we could also remove this function by introducing a `SilentPaymentsSign` function (similar to how we have a taproot sign function). I'll dig into this a bit more, but it seems better to have a special signing function that can _only_ be used for silent payments, instead of a generic `CKey::TweakAdd` function that might be abused/footgunny.
💬 josibake commented on pull request "Silent Payments: Receiving":
(https://github.com/bitcoin/bitcoin/pull/32966#discussion_r2325275304)
This is only used to recreate the silent payment taproot scriptpubkeys, i.e., $B_{spend} + tweak \cdot G$. But since we already have the full scriptpubkey when scanning, we could just save the tweak and scriptpubkey to the database.
This means more storage, but then we can drop this `CPubKey::TweakAdd` method. This seems better to me because storage is cheap, this should make loading the wallet faster, and it removes a footgun from CPubKey (we dont really want to have a generic method for add
...
(https://github.com/bitcoin/bitcoin/pull/32966#discussion_r2325275304)
This is only used to recreate the silent payment taproot scriptpubkeys, i.e., $B_{spend} + tweak \cdot G$. But since we already have the full scriptpubkey when scanning, we could just save the tweak and scriptpubkey to the database.
This means more storage, but then we can drop this `CPubKey::TweakAdd` method. This seems better to me because storage is cheap, this should make loading the wallet faster, and it removes a footgun from CPubKey (we dont really want to have a generic method for add
...
💬 josibake commented on pull request "Silent Payments: Receiving":
(https://github.com/bitcoin/bitcoin/pull/32966#discussion_r2325303247)
Not a comment for this commit specifically, but I do think we should document either in a commit or in a commit message the entire logic and flow of using silent payments for change. Its a bit complicated and without a clear understanding of all of the cases we need to cover, these changes are a bit hard to follow.
(https://github.com/bitcoin/bitcoin/pull/32966#discussion_r2325303247)
Not a comment for this commit specifically, but I do think we should document either in a commit or in a commit message the entire logic and flow of using silent payments for change. Its a bit complicated and without a clear understanding of all of the cases we need to cover, these changes are a bit hard to follow.
💬 josibake commented on pull request "Silent Payments: Receiving":
(https://github.com/bitcoin/bitcoin/pull/32966#discussion_r2325262802)
Revisiting this, it would be nice to if we enforce that an SP descriptor cannot be multipath. Beyond that, I think its fine to keep the plurals and for loops, since that matches the style of the rest of the wallet code.
(https://github.com/bitcoin/bitcoin/pull/32966#discussion_r2325262802)
Revisiting this, it would be nice to if we enforce that an SP descriptor cannot be multipath. Beyond that, I think its fine to keep the plurals and for loops, since that matches the style of the rest of the wallet code.
💬 josibake commented on pull request "Silent Payments: Receiving":
(https://github.com/bitcoin/bitcoin/pull/32966#discussion_r2325305066)
Even better, if we dont have one already, we can create a test case for all of the possible change scenarios and document there.
(https://github.com/bitcoin/bitcoin/pull/32966#discussion_r2325305066)
Even better, if we dont have one already, we can create a test case for all of the possible change scenarios and document there.
💬 josibake commented on pull request "Silent Payments: Receiving":
(https://github.com/bitcoin/bitcoin/pull/32966#discussion_r2325296901)
Not quite sure why this is necessary?
(https://github.com/bitcoin/bitcoin/pull/32966#discussion_r2325296901)
Not quite sure why this is necessary?
💬 josibake commented on pull request "Silent Payments: Receiving":
(https://github.com/bitcoin/bitcoin/pull/32966#discussion_r2325281936)
Reiterating this point. I think I've seen a few PRs related to undodata recently, seems like a good time to bundle all of them as a well motivated refactor? cc @TheCharlatan
(https://github.com/bitcoin/bitcoin/pull/32966#discussion_r2325281936)
Reiterating this point. I think I've seen a few PRs related to undodata recently, seems like a good time to bundle all of them as a well motivated refactor? cc @TheCharlatan
🤔 josibake reviewed a pull request: "Silent Payments: Receiving"
(https://github.com/bitcoin/bitcoin/pull/32966#pullrequestreview-3189588826)
Looking good! Really nice to see you were able to remove a lot of custom code related to how we handle the scan and spend keys. Left some comments/questions below.
(https://github.com/bitcoin/bitcoin/pull/32966#pullrequestreview-3189588826)
Looking good! Really nice to see you were able to remove a lot of custom code related to how we handle the scan and spend keys. Left some comments/questions below.
💬 Eunovo commented on pull request "Silent Payments: Receiving":
(https://github.com/bitcoin/bitcoin/pull/32966#discussion_r2325317749)
Maybe we can use a namespace?
(https://github.com/bitcoin/bitcoin/pull/32966#discussion_r2325317749)
Maybe we can use a namespace?
💬 Eunovo commented on pull request "Silent Payments: Receiving":
(https://github.com/bitcoin/bitcoin/pull/32966#discussion_r2325328988)
We need it to extract the `change_dest` from the results of `CreateSilentPaymentOutputs`.
(https://github.com/bitcoin/bitcoin/pull/32966#discussion_r2325328988)
We need it to extract the `change_dest` from the results of `CreateSilentPaymentOutputs`.
📝 stickies-v opened a pull request: "kernel: make blockTip index const"
(https://github.com/bitcoin/bitcoin/pull/33321)
Notification interface subscribers need to view, but not mutate, the index.
This change allows improving the #30595 kernel interface, see e.g. `BlockTreeEntry` where [currently](https://github.com/bitcoin/bitcoin/pull/30595/files#diff-4d05cd02fdce641be603f0f9abcecfeaf76944285d4539ba4bbc40337fa9bbc2R617) a `View` is constructed from a non-const pointer, whereas really this should be a `const btck_BlockTreeEntry* entry`.
(https://github.com/bitcoin/bitcoin/pull/33321)
Notification interface subscribers need to view, but not mutate, the index.
This change allows improving the #30595 kernel interface, see e.g. `BlockTreeEntry` where [currently](https://github.com/bitcoin/bitcoin/pull/30595/files#diff-4d05cd02fdce641be603f0f9abcecfeaf76944285d4539ba4bbc40337fa9bbc2R617) a `View` is constructed from a non-const pointer, whereas really this should be a `const btck_BlockTreeEntry* entry`.