💬 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`.
💬 mzumsande commented on issue "InitError() doesn't always halt node startup when blockchain state exists":
(https://github.com/bitcoin/bitcoin/issues/33276#issuecomment-3258705593)
Is this maybe more about wording/definitions? I'd say node startup is finished when RPC is enabled and we log "[Done Loading](https://github.com/bitcoin/bitcoin/blob/37c21ebe4078d5a7b9d8a91aca2ab8b118b9d69f/src/init.cpp#L2157C4-L2157C48)" - in the log, the node does halt before that would happen.
It's true (and could be unexpected) that `initload` may do some work connecting blocks before that, but is there a downside to that? It's not possible to simply delay the initload start without other c
...
(https://github.com/bitcoin/bitcoin/issues/33276#issuecomment-3258705593)
Is this maybe more about wording/definitions? I'd say node startup is finished when RPC is enabled and we log "[Done Loading](https://github.com/bitcoin/bitcoin/blob/37c21ebe4078d5a7b9d8a91aca2ab8b118b9d69f/src/init.cpp#L2157C4-L2157C48)" - in the log, the node does halt before that would happen.
It's true (and could be unexpected) that `initload` may do some work connecting blocks before that, but is there a downside to that? It's not possible to simply delay the initload start without other c
...
👍 TheCharlatan approved a pull request: "kernel: make blockTip index const"
(https://github.com/bitcoin/bitcoin/pull/33321#pullrequestreview-3189781820)
ACK 75d9b72475708ee0da13fb23ef65dcced805b6af
(https://github.com/bitcoin/bitcoin/pull/33321#pullrequestreview-3189781820)
ACK 75d9b72475708ee0da13fb23ef65dcced805b6af
💬 josibake commented on pull request "Silent Payments: sending":
(https://github.com/bitcoin/bitcoin/pull/28201#discussion_r2325436395)
Thanks for finding this! I've rebased on master and pulled in the latest libsecp256k1 changes; I'll push a fix for this tomorrow.
(https://github.com/bitcoin/bitcoin/pull/28201#discussion_r2325436395)
Thanks for finding this! I've rebased on master and pulled in the latest libsecp256k1 changes; I'll push a fix for this tomorrow.
💬 l0rinc commented on pull request "headerssync: Correct unrealistic unit test behavior":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2325496639)
You can resolve it if you feel strongly about it, it's not a blocker from my side
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2325496639)
You can resolve it if you feel strongly about it, it's not a blocker from my side
💬 l0rinc commented on pull request "headerssync: Correct unrealistic unit test behavior":
(https://github.com/bitcoin/bitcoin/pull/32579#issuecomment-3258918462)
reACK 33d550d3044f9075cc866093c453158288f12dec
Would be great if @sipa or @sdaftuar could also take a look.
(https://github.com/bitcoin/bitcoin/pull/32579#issuecomment-3258918462)
reACK 33d550d3044f9075cc866093c453158288f12dec
Would be great if @sipa or @sdaftuar could also take a look.
🤔 marcofleon reviewed a pull request: "contrib: update fixed seeds"
(https://github.com/bitcoin/bitcoin/pull/33283#pullrequestreview-3189994622)
ACK 939678940f6c3fdbc36d57a9c9ef6f8edf89d065
Changes in the `makeseeds.py` script look fine. I ran the script myself and the direct diff isn't super useful, but it produced a similar number of seeds at least.
(https://github.com/bitcoin/bitcoin/pull/33283#pullrequestreview-3189994622)
ACK 939678940f6c3fdbc36d57a9c9ef6f8edf89d065
Changes in the `makeseeds.py` script look fine. I ran the script myself and the direct diff isn't super useful, but it produced a similar number of seeds at least.
👍 l0rinc approved a pull request: "kernel: make blockTip index const"
(https://github.com/bitcoin/bitcoin/pull/33321#pullrequestreview-3189998416)
Code review ACK 75d9b72475708ee0da13fb23ef65dcced805b6af
(https://github.com/bitcoin/bitcoin/pull/33321#pullrequestreview-3189998416)
Code review ACK 75d9b72475708ee0da13fb23ef65dcced805b6af
💬 Sjors commented on pull request "Add functional test for IPC interface":
(https://github.com/bitcoin/bitcoin/pull/33201#issuecomment-3258956684)
ACK a341e11ac92b30aac856049c696a9ac39854569d
As if by magic, `interface_ipc.py` now _does_ run on the test each commit job.
(https://github.com/bitcoin/bitcoin/pull/33201#issuecomment-3258956684)
ACK a341e11ac92b30aac856049c696a9ac39854569d
As if by magic, `interface_ipc.py` now _does_ run on the test each commit job.
💬 Sjors commented on pull request "multiprocess: Don't require bitcoin -m argument when IPC options are used":
(https://github.com/bitcoin/bitcoin/pull/33229#discussion_r2325532799)
I asked because it triggered a missing documentation error.
(https://github.com/bitcoin/bitcoin/pull/33229#discussion_r2325532799)
I asked because it triggered a missing documentation error.
💬 Sjors commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#issuecomment-3258979508)
Meanwhile I managed to test a script path spend with Ledger as well (though I haven't tried MuSig2 inside a script path), using https://github.com/bitcoin-core/HWI/pull/794. With that I'm pretty happy with interoperability.
Will do another code review round soon(tm).
(https://github.com/bitcoin/bitcoin/pull/29675#issuecomment-3258979508)
Meanwhile I managed to test a script path spend with Ledger as well (though I haven't tried MuSig2 inside a script path), using https://github.com/bitcoin-core/HWI/pull/794. With that I'm pretty happy with interoperability.
Will do another code review round soon(tm).
💬 stickies-v commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2325351750)
I think this should be a `const btck_BlockTreeEntry* entry`, so I've opened https://github.com/bitcoin/bitcoin/pull/33321 for the upstream change
```suggestion
typedef void (*btck_NotifyBlockTip)(void* user_data, btck_SynchronizationState state, const btck_BlockTreeEntry* entry, double verification_progress);
```
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2325351750)
I think this should be a `const btck_BlockTreeEntry* entry`, so I've opened https://github.com/bitcoin/bitcoin/pull/33321 for the upstream change
```suggestion
typedef void (*btck_NotifyBlockTip)(void* user_data, btck_SynchronizationState state, const btck_BlockTreeEntry* entry, double verification_progress);
```
💬 stickies-v commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2325458060)
```suggestion
auto block{std::make_shared<CBlock>()};
```
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2325458060)
```suggestion
auto block{std::make_shared<CBlock>()};
```
💬 stickies-v commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2325447195)
I think `::create` and `::copy` methods could further improve the implementation, e.g.:
<details>
<summary>git diff on 82c503641a</summary>
```diff
diff --git a/src/kernel/bitcoinkernel.cpp b/src/kernel/bitcoinkernel.cpp
index fc927ea3d7..51d914eaa1 100644
--- a/src/kernel/bitcoinkernel.cpp
+++ b/src/kernel/bitcoinkernel.cpp
@@ -87,6 +87,19 @@ struct Handle {
{
delete reinterpret_cast<CPP*>(ptr);
}
+
+ template <typename... Args>
+ static C* create(Args
...
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2325447195)
I think `::create` and `::copy` methods could further improve the implementation, e.g.:
<details>
<summary>git diff on 82c503641a</summary>
```diff
diff --git a/src/kernel/bitcoinkernel.cpp b/src/kernel/bitcoinkernel.cpp
index fc927ea3d7..51d914eaa1 100644
--- a/src/kernel/bitcoinkernel.cpp
+++ b/src/kernel/bitcoinkernel.cpp
@@ -87,6 +87,19 @@ struct Handle {
{
delete reinterpret_cast<CPP*>(ptr);
}
+
+ template <typename... Args>
+ static C* create(Args
...
💬 stickies-v commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2325502582)
I think this distinction is irrelevant: if chain is always unowned, this function can never be called, because the caller would never have a non-const `btck_Chain*`, in which case we can probably just remove the function, or keep it for completeness because it's harmless. Alternatively, and preferably, let's just add a `Handler::destroy` function, so we can easily change the logic for all destroy functions later (e.g. add logging if nullptr was passed, which may be helpful for debugging):
<de
...
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2325502582)
I think this distinction is irrelevant: if chain is always unowned, this function can never be called, because the caller would never have a non-const `btck_Chain*`, in which case we can probably just remove the function, or keep it for completeness because it's harmless. Alternatively, and preferably, let's just add a `Handler::destroy` function, so we can easily change the logic for all destroy functions later (e.g. add logging if nullptr was passed, which may be helpful for debugging):
<de
...