💬 Sjors commented on pull request "Have createNewBlock() wait for tip, make rpc handle shutdown during long poll and wait methods":
(https://github.com/bitcoin/bitcoin/pull/31785#discussion_r1954295417)
Indeed, but on the other hand code changes over time, stuff appears between the if statement and the usage of `tip`, then someone changes the if statement, and now we have a crash...
(https://github.com/bitcoin/bitcoin/pull/31785#discussion_r1954295417)
Indeed, but on the other hand code changes over time, stuff appears between the if statement and the usage of `tip`, then someone changes the if statement, and now we have a crash...
💬 vasild commented on pull request "Have createNewBlock() wait for tip, make rpc handle shutdown during long poll and wait methods":
(https://github.com/bitcoin/bitcoin/pull/31785#discussion_r1954297095)
Yes, this scenario I had in mind.
> in that case to ignore the timeout and instead keep waiting for the node to set a tip
Hmm. What if the tip is not being connected for whatever reason? Wait forever? The current behavior of respecting the timeout looks safer. Would be an odd user experience to provide a timeout and the program to decide to wait longer for whatever reason.
(https://github.com/bitcoin/bitcoin/pull/31785#discussion_r1954297095)
Yes, this scenario I had in mind.
> in that case to ignore the timeout and instead keep waiting for the node to set a tip
Hmm. What if the tip is not being connected for whatever reason? Wait forever? The current behavior of respecting the timeout looks safer. Would be an odd user experience to provide a timeout and the program to decide to wait longer for whatever reason.
💬 Sjors commented on pull request "Have createNewBlock() wait for tip, make rpc handle shutdown during long poll and wait methods":
(https://github.com/bitcoin/bitcoin/pull/31785#discussion_r1954298757)
It seems the compiler doesn't know about this default inside the implementation file.
(https://github.com/bitcoin/bitcoin/pull/31785#discussion_r1954298757)
It seems the compiler doesn't know about this default inside the implementation file.
💬 vasild commented on pull request "Have createNewBlock() wait for tip, make rpc handle shutdown during long poll and wait methods":
(https://github.com/bitcoin/bitcoin/pull/31785#discussion_r1954300543)
Ok
(https://github.com/bitcoin/bitcoin/pull/31785#discussion_r1954300543)
Ok
💬 willcl-ark commented on pull request "chore: remove redundant word":
(https://github.com/bitcoin/bitcoin/pull/31855#issuecomment-2656266441)
Thanks for the contribution, but this should probably be closed.
In this project we prefer typos to be fixed organically as that area of the codebase is worked on to avoid diluting precious developer review time. See our https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#pull-request-philosophy
(https://github.com/bitcoin/bitcoin/pull/31855#issuecomment-2656266441)
Thanks for the contribution, but this should probably be closed.
In this project we prefer typos to be fixed organically as that area of the codebase is worked on to avoid diluting precious developer review time. See our https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#pull-request-philosophy
💬 Sjors commented on pull request "Have createNewBlock() wait for tip, make rpc handle shutdown during long poll and wait methods":
(https://github.com/bitcoin/bitcoin/pull/31785#issuecomment-2656278649)
I changed `waitTipChanged()` to only return `nullopt` during shutdown. This is achieved by extending the `timeout` in the unlikely event that node initialization takes longer.
I introduced a new `RPC_SHUTDOWN_ERROR` code and use it in case `waitfornewblock` and friends are called really early and the node shuts down (probably very hard to reach in practice). See https://github.com/bitcoin/bitcoin/pull/31785#discussion_r1952373326
On question about long polling I still need to investigate:
...
(https://github.com/bitcoin/bitcoin/pull/31785#issuecomment-2656278649)
I changed `waitTipChanged()` to only return `nullopt` during shutdown. This is achieved by extending the `timeout` in the unlikely event that node initialization takes longer.
I introduced a new `RPC_SHUTDOWN_ERROR` code and use it in case `waitfornewblock` and friends are called really early and the node shuts down (probably very hard to reach in practice). See https://github.com/bitcoin/bitcoin/pull/31785#discussion_r1952373326
On question about long polling I still need to investigate:
...
💬 Sjors commented on pull request "Have createNewBlock() wait for tip, make rpc handle shutdown during long poll and wait methods":
(https://github.com/bitcoin/bitcoin/pull/31785#discussion_r1954310804)
I ended up with something similar...
(https://github.com/bitcoin/bitcoin/pull/31785#discussion_r1954310804)
I ended up with something similar...
💬 fanquake commented on pull request "ci: switch MSAN to use prebuilt Clang binaries":
(https://github.com/bitcoin/bitcoin/pull/31850#issuecomment-2656283257)
Swapped to rc2. Needs a fixup for MSAN fuzz.
(https://github.com/bitcoin/bitcoin/pull/31850#issuecomment-2656283257)
Swapped to rc2. Needs a fixup for MSAN fuzz.
💬 willcl-ark commented on issue "Wallet passpharse":
(https://github.com/bitcoin/bitcoin/issues/31852#issuecomment-2656290307)
> but now it doesn’t work as usual
Which previous version(s) of bitcoin core did this particular wallet and password work with?
Does it still work with that version (thereby implying a regression in newer versions)?
(https://github.com/bitcoin/bitcoin/issues/31852#issuecomment-2656290307)
> but now it doesn’t work as usual
Which previous version(s) of bitcoin core did this particular wallet and password work with?
Does it still work with that version (thereby implying a regression in newer versions)?
💬 willcl-ark commented on pull request "ci: switch MSAN to use prebuilt Clang binaries":
(https://github.com/bitcoin/bitcoin/pull/31850#issuecomment-2656293223)
For fuzzing end-users, you might consider touching up the wording in:
https://github.com/bitcoin/bitcoin/blob/55cf39e4c54da6639a8f1f7c813c2909454cada1/doc/fuzzing.md?plain=1#L104-L112
...to directly mention using pre-built llvm bins (although it does already say to use the CI job as a reference which is and will remain accurate).
(https://github.com/bitcoin/bitcoin/pull/31850#issuecomment-2656293223)
For fuzzing end-users, you might consider touching up the wording in:
https://github.com/bitcoin/bitcoin/blob/55cf39e4c54da6639a8f1f7c813c2909454cada1/doc/fuzzing.md?plain=1#L104-L112
...to directly mention using pre-built llvm bins (although it does already say to use the CI job as a reference which is and will remain accurate).
👍 maflcko approved a pull request: "logging: Ensure -debug=0/none behaves consistently with -nodebug"
(https://github.com/bitcoin/bitcoin/pull/31767#pullrequestreview-2614664083)
left two nits
review ACK 7afeaa24693039730c9ae00c325c2b9d0e2deda1 👡
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review
...
(https://github.com/bitcoin/bitcoin/pull/31767#pullrequestreview-2614664083)
left two nits
review ACK 7afeaa24693039730c9ae00c325c2b9d0e2deda1 👡
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review
...
💬 maflcko commented on pull request "logging: Ensure -debug=0/none behaves consistently with -nodebug":
(https://github.com/bitcoin/bitcoin/pull/31767#discussion_r1954308137)
Nit in a8fedb36a71ac193fc158284b14d4eb474e5ab62: Why is this needed? It seems like a no-op:
* If the list of debug args is empty, the code below can handle it. In fact, it must handle the case anyway. For example, if someone passes just `-nodebug`, `IsArgSet` will be true, but `GetArgs` will be empty.
* If the list of debug args is not empty, it is just a redundant check?
So my recommendation would be to remove it. If you want, you can keep the `{}` for the scope.
(https://github.com/bitcoin/bitcoin/pull/31767#discussion_r1954308137)
Nit in a8fedb36a71ac193fc158284b14d4eb474e5ab62: Why is this needed? It seems like a no-op:
* If the list of debug args is empty, the code below can handle it. In fact, it must handle the case anyway. For example, if someone passes just `-nodebug`, `IsArgSet` will be true, but `GetArgs` will be empty.
* If the list of debug args is not empty, it is just a redundant check?
So my recommendation would be to remove it. If you want, you can keep the `{}` for the scope.
💬 maflcko commented on pull request "logging: Ensure -debug=0/none behaves consistently with -nodebug":
(https://github.com/bitcoin/bitcoin/pull/31767#discussion_r1954318238)
Nit in https://github.com/bitcoin/bitcoin/commit/a8fedb36a71ac193fc158284b14d4eb474e5ab62: Would be nice to explain why this is done: Something like: `// GetArgs disregards any debugging categories appearing before -nodebug, so do the same for -debug=0/none`
(https://github.com/bitcoin/bitcoin/pull/31767#discussion_r1954318238)
Nit in https://github.com/bitcoin/bitcoin/commit/a8fedb36a71ac193fc158284b14d4eb474e5ab62: Would be nice to explain why this is done: Something like: `// GetArgs disregards any debugging categories appearing before -nodebug, so do the same for -debug=0/none`
💬 fanquake commented on pull request "net: reduce CAddress usage to CService or CNetAddr":
(https://github.com/bitcoin/bitcoin/pull/31854#discussion_r1954320762)
Still documented as returning a `CAddress`?
(https://github.com/bitcoin/bitcoin/pull/31854#discussion_r1954320762)
Still documented as returning a `CAddress`?
💬 willcl-ark commented on pull request "test, tracing: don't use problematic `bpf_usdt_readarg_p()`":
(https://github.com/bitcoin/bitcoin/pull/31848#issuecomment-2656312092)
Approach ACK
So if I understand the change correctly, the new (documented/recommended) behaviour is to always first `read_arg()` which gets the memory location of the data, then `read_user[_str]()` which then actually performs the copy from user memory to the BPF memory space.
If I have this correct, then using the documented, seemingly more-reliable, and more explicit approach seems fine to me.
Not sure how I could test the reliability of this myself, but https://github.com/bitcoin/bit
...
(https://github.com/bitcoin/bitcoin/pull/31848#issuecomment-2656312092)
Approach ACK
So if I understand the change correctly, the new (documented/recommended) behaviour is to always first `read_arg()` which gets the memory location of the data, then `read_user[_str]()` which then actually performs the copy from user memory to the BPF memory space.
If I have this correct, then using the documented, seemingly more-reliable, and more explicit approach seems fine to me.
Not sure how I could test the reliability of this myself, but https://github.com/bitcoin/bit
...
💬 vasild commented on pull request "Have createNewBlock() wait for tip, make rpc handle shutdown during long poll and wait methods":
(https://github.com/bitcoin/bitcoin/pull/31785#discussion_r1954339722)
TIL https://flylib.com/books/en/2.887.1.53/1/
Possible solutions I see:
* drop the default altogether and have all callers use 2 arguments
* use the "non-virtual interface idiom" as described in the chapter above (it is from the book "Effective C++" by Scott Meyers)
* use overload instead of default arguments. Have two methods `waitTipChanged(hash)` and `waitTipChanged(hash, timeout)` (the first could call the second internally).
(https://github.com/bitcoin/bitcoin/pull/31785#discussion_r1954339722)
TIL https://flylib.com/books/en/2.887.1.53/1/
Possible solutions I see:
* drop the default altogether and have all callers use 2 arguments
* use the "non-virtual interface idiom" as described in the chapter above (it is from the book "Effective C++" by Scott Meyers)
* use overload instead of default arguments. Have two methods `waitTipChanged(hash)` and `waitTipChanged(hash, timeout)` (the first could call the second internally).
💬 fanquake commented on pull request "cmake: Install man pages for configured targets only":
(https://github.com/bitcoin/bitcoin/pull/31765#issuecomment-2656330751)
Close this given #31844 (which also solves #31745)? Or, it'd be good if you could comment either here or there on the alternative approach.
(https://github.com/bitcoin/bitcoin/pull/31765#issuecomment-2656330751)
Close this given #31844 (which also solves #31745)? Or, it'd be good if you could comment either here or there on the alternative approach.
💬 vasild commented on pull request "net: reduce CAddress usage to CService or CNetAddr":
(https://github.com/bitcoin/bitcoin/pull/31854#issuecomment-2656335347)
`74f42a5920...cd4bfaee10`: address https://github.com/bitcoin/bitcoin/pull/31854#discussion_r1954320762
(https://github.com/bitcoin/bitcoin/pull/31854#issuecomment-2656335347)
`74f42a5920...cd4bfaee10`: address https://github.com/bitcoin/bitcoin/pull/31854#discussion_r1954320762
💬 vasild commented on pull request "net: reduce CAddress usage to CService or CNetAddr":
(https://github.com/bitcoin/bitcoin/pull/31854#discussion_r1954343810)
Should be "CService", fixed, thanks!
(https://github.com/bitcoin/bitcoin/pull/31854#discussion_r1954343810)
Should be "CService", fixed, thanks!
💬 InnDe commented on issue "Wallet passpharse":
(https://github.com/bitcoin/bitcoin/issues/31852#issuecomment-2656343492)
I think 14 or 15 that was before 12 years
On Thu, 13 Feb 2025 at 14:20 Will Clark ***@***.***> wrote:
> but now it doesn’t work as usual
>
> Which previous version(s) of bitcoin core did this particular wallet and
> password work with?
>
> Does it still work with that version (thereby implying a regression in
> newer versions)?
>
> —
> Reply to this email directly, view it on GitHub
> <https://github.com/bitcoin/bitcoin/issues/31852#issuecomment-2656290307>,
> or unsubscribe
> <h
...
(https://github.com/bitcoin/bitcoin/issues/31852#issuecomment-2656343492)
I think 14 or 15 that was before 12 years
On Thu, 13 Feb 2025 at 14:20 Will Clark ***@***.***> wrote:
> but now it doesn’t work as usual
>
> Which previous version(s) of bitcoin core did this particular wallet and
> password work with?
>
> Does it still work with that version (thereby implying a regression in
> newer versions)?
>
> —
> Reply to this email directly, view it on GitHub
> <https://github.com/bitcoin/bitcoin/issues/31852#issuecomment-2656290307>,
> or unsubscribe
> <h
...