💬 mzumsande commented on pull request "rpc,net: Add getpeerbyid and listpeersbyids RPCs":
(https://github.com/bitcoin/bitcoin/pull/32972#issuecomment-3073944718)
I also prefer the current approach from #32741 to this - while users can always just filter `getpeerinfo` output with jq or other tools I can see how that is inconvenient for some - but introducing multiple new RPCs for this seems overkill to me.
(https://github.com/bitcoin/bitcoin/pull/32972#issuecomment-3073944718)
I also prefer the current approach from #32741 to this - while users can always just filter `getpeerinfo` output with jq or other tools I can see how that is inconvenient for some - but introducing multiple new RPCs for this seems overkill to me.
💬 fanquake commented on pull request "doc: add alpine build instructions":
(https://github.com/bitcoin/bitcoin/pull/32559#discussion_r2207734166)
> Is xz really necessary?
It's currently pulled in as a cmake dep, but it also seems fine to be explicit.
> and install samurai.
Yea, I think we should use `samurai` here.
(https://github.com/bitcoin/bitcoin/pull/32559#discussion_r2207734166)
> Is xz really necessary?
It's currently pulled in as a cmake dep, but it also seems fine to be explicit.
> and install samurai.
Yea, I think we should use `samurai` here.
👍 pinheadmz approved a pull request: "net: Fix Discover() not running when using -bind=0.0.0.0:port"
(https://github.com/bitcoin/bitcoin/pull/32757#pullrequestreview-3020667324)
ACK ad3f60b0ea59dbbe7aedcfdcca1caddefffe6976
Built and tested on macos/arm64 and debian/x86. Reviewed code changes which are minimal, this PR searches bind settings for 0.0.0.0 specifically to call Discover() which attempts to add local addresses for peer advertising. Also adds test coverage using `1.1.1.1` and `2.2.2.2` which I was able to set up and check locally on both platforms. Also tested on main.
I would really like to know if its possible to run this test in CI as [vasild suggeste
...
(https://github.com/bitcoin/bitcoin/pull/32757#pullrequestreview-3020667324)
ACK ad3f60b0ea59dbbe7aedcfdcca1caddefffe6976
Built and tested on macos/arm64 and debian/x86. Reviewed code changes which are minimal, this PR searches bind settings for 0.0.0.0 specifically to call Discover() which attempts to add local addresses for peer advertising. Also adds test coverage using `1.1.1.1` and `2.2.2.2` which I was able to set up and check locally on both platforms. Also tested on main.
I would really like to know if its possible to run this test in CI as [vasild suggeste
...
💬 pinheadmz commented on pull request "net: Fix Discover() not running when using -bind=0.0.0.0:port":
(https://github.com/bitcoin/bitcoin/pull/32757#discussion_r2207665334)
The variable `local` in these lines is being used outside the `for` loop which defines it. If `localaddresses` is empty `[]` the error message itself will cause an error.
(https://github.com/bitcoin/bitcoin/pull/32757#discussion_r2207665334)
The variable `local` in these lines is being used outside the `for` loop which defines it. If `localaddresses` is empty `[]` the error message itself will cause an error.
💬 fanquake commented on pull request "threading: remove ancient CRITICAL_SECTION macros":
(https://github.com/bitcoin/bitcoin/pull/32592#issuecomment-3073973824)
Put this on `30.0`.
(https://github.com/bitcoin/bitcoin/pull/32592#issuecomment-3073973824)
Put this on `30.0`.
💬 fanquake commented on pull request "doc: add alpine build instructions":
(https://github.com/bitcoin/bitcoin/pull/32559#discussion_r2207801898)
Did using the tracepoints work? Depends and a build will likely succeed, depends is just copying a header, and the CMake existence check is basic.
(https://github.com/bitcoin/bitcoin/pull/32559#discussion_r2207801898)
Did using the tracepoints work? Depends and a build will likely succeed, depends is just copying a header, and the CMake existence check is basic.
💬 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