π¬ l0rinc commented on pull request "log: Use more severe log level (warn/err) where appropriate":
(https://github.com/bitcoin/bitcoin/pull/33960#discussion_r2568173125)
hmmm, are you sure this is an error?
(https://github.com/bitcoin/bitcoin/pull/33960#discussion_r2568173125)
hmmm, are you sure this is an error?
π¬ l0rinc commented on pull request "log: Use more severe log level (warn/err) where appropriate":
(https://github.com/bitcoin/bitcoin/pull/33960#discussion_r2568193209)
do we need to keep the `WARNING` prefix here?
```suggestion
LogWarning("SQLite is configured to not wait for data to be flushed to disk. Data loss and corruption may occur.");
```
(https://github.com/bitcoin/bitcoin/pull/33960#discussion_r2568193209)
do we need to keep the `WARNING` prefix here?
```suggestion
LogWarning("SQLite is configured to not wait for data to be flushed to disk. Data loss and corruption may occur.");
```
π¬ l0rinc commented on pull request "log: Use more severe log level (warn/err) where appropriate":
(https://github.com/bitcoin/bitcoin/pull/33960#discussion_r2568197167)
```suggestion
LogWarning("Request rejected because http work queue depth exceeded, it can be increased with the -rpcworkqueue= setting");
```
(https://github.com/bitcoin/bitcoin/pull/33960#discussion_r2568197167)
```suggestion
LogWarning("Request rejected because http work queue depth exceeded, it can be increased with the -rpcworkqueue= setting");
```
π¬ l0rinc commented on pull request "log: Use more severe log level (warn/err) where appropriate":
(https://github.com/bitcoin/bitcoin/pull/33960#discussion_r2568179747)
If it's`Fatal`, shouldn't it be a `LogError`?
(https://github.com/bitcoin/bitcoin/pull/33960#discussion_r2568179747)
If it's`Fatal`, shouldn't it be a `LogError`?
π¬ l0rinc commented on pull request "log: Use more severe log level (warn/err) where appropriate":
(https://github.com/bitcoin/bitcoin/pull/33960#discussion_r2568198946)
nit:
```suggestion
LogWarning("Unhandled HTTP request");
```
(https://github.com/bitcoin/bitcoin/pull/33960#discussion_r2568198946)
nit:
```suggestion
LogWarning("Unhandled HTTP request");
```
π¬ l0rinc commented on pull request "log: Use more severe log level (warn/err) where appropriate":
(https://github.com/bitcoin/bitcoin/pull/33960#discussion_r2568203370)
```suggestion
LogWarning("Cursor closed but could not finalize cursor statement: %s",
```
(https://github.com/bitcoin/bitcoin/pull/33960#discussion_r2568203370)
```suggestion
LogWarning("Cursor closed but could not finalize cursor statement: %s",
```
π¬ l0rinc commented on pull request "log: Use more severe log level (warn/err) where appropriate":
(https://github.com/bitcoin/bitcoin/pull/33960#discussion_r2568217706)
So is this a warning or an error?
(https://github.com/bitcoin/bitcoin/pull/33960#discussion_r2568217706)
So is this a warning or an error?
π¬ m3dwards commented on pull request "depends, doc: Learn `x86_64-w64-mingw32ucrt` host and document it":
(https://github.com/bitcoin/bitcoin/pull/33857#issuecomment-3585450300)
> I can confirm the issue on `aarch64`
According to the Debian bug report, seems it's also an issue on RISC-V.
(https://github.com/bitcoin/bitcoin/pull/33857#issuecomment-3585450300)
> I can confirm the issue on `aarch64`
According to the Debian bug report, seems it's also an issue on RISC-V.
π sedited approved a pull request: "kernel: Add block header support and validation"
(https://github.com/bitcoin/bitcoin/pull/33822#pullrequestreview-3514976734)
ACK 10389bcfcf3eb92a08ff1fafab86db89749520c2
(https://github.com/bitcoin/bitcoin/pull/33822#pullrequestreview-3514976734)
ACK 10389bcfcf3eb92a08ff1fafab86db89749520c2
π¬ sedited commented on pull request "kernel: Add block header support and validation":
(https://github.com/bitcoin/bitcoin/pull/33822#discussion_r2568377264)
Nit (clang-format-diff): Needs a whitespace after `template`.
(https://github.com/bitcoin/bitcoin/pull/33822#discussion_r2568377264)
Nit (clang-format-diff): Needs a whitespace after `template`.
π¬ sedited commented on pull request "kernel: Add block header support and validation":
(https://github.com/bitcoin/bitcoin/pull/33822#discussion_r2568398257)
Nit (clang-format-diff): Missing whitespace to align the comment close.
(https://github.com/bitcoin/bitcoin/pull/33822#discussion_r2568398257)
Nit (clang-format-diff): Missing whitespace to align the comment close.
π¬ sedited commented on pull request "kernel: Add block header support and validation":
(https://github.com/bitcoin/bitcoin/pull/33822#discussion_r2568391430)
Nit (clang-format-diff): Missing whitespace.
(https://github.com/bitcoin/bitcoin/pull/33822#discussion_r2568391430)
Nit (clang-format-diff): Missing whitespace.
π¬ optout21 commented on pull request "merkle: preβreserve leaves to prevent reallocs with odd vtx count":
(https://github.com/bitcoin/bitcoin/pull/32497#discussion_r2568419746)
Hmm, I see this as new code added by this commit, not present in the original.
(https://github.com/bitcoin/bitcoin/pull/32497#discussion_r2568419746)
Hmm, I see this as new code added by this commit, not present in the original.
π€ rkrux requested changes to a pull request: "test: refactor mempool_accept_wtxid"
(https://github.com/bitcoin/bitcoin/pull/33067#pullrequestreview-3515035765)
Thanks for addressing the comments earlier.
> This PR improves mempool_accept_wtxid.py by using a pre-mined chain instead of generating new blocks, switching to MiniWallet to avoid RPC calls for signing transactions
I haven't timed the test yet but if some test latency improvement has been noticed (ref: https://github.com/bitcoin/bitcoin/pull/33067#pullrequestreview-3079794265), then it would be helpful to highlight it in the PR description imo.
(https://github.com/bitcoin/bitcoin/pull/33067#pullrequestreview-3515035765)
Thanks for addressing the comments earlier.
> This PR improves mempool_accept_wtxid.py by using a pre-mined chain instead of generating new blocks, switching to MiniWallet to avoid RPC calls for signing transactions
I haven't timed the test yet but if some test latency improvement has been noticed (ref: https://github.com/bitcoin/bitcoin/pull/33067#pullrequestreview-3079794265), then it would be helpful to highlight it in the PR description imo.
π¬ rkrux commented on pull request "test: refactor mempool_accept_wtxid":
(https://github.com/bitcoin/bitcoin/pull/33067#discussion_r2568435169)
Nit: It would be nice to calculate these amounts based on the wallet balance or the utxo being spent by the wallet instead of hardcoding that relies on some MiniWallet assumptions.
(https://github.com/bitcoin/bitcoin/pull/33067#discussion_r2568435169)
Nit: It would be nice to calculate these amounts based on the wallet balance or the utxo being spent by the wallet instead of hardcoding that relies on some MiniWallet assumptions.
π¬ rkrux commented on pull request "test: refactor mempool_accept_wtxid":
(https://github.com/bitcoin/bitcoin/pull/33067#discussion_r2568423936)
I feel there is scope for generalising this function further. Similar to how the previous class implementation used only the `CTransaction` class and the like.
Currently, this function is tied to the `MiniWallet` that I don't think is necessary. A more generic form of this function should be able to be used by the usual RPC wallets as well.
Consider the following diff that puts the responsibility of building the base parent transaction and submitting the final parent transaction to the caller
...
(https://github.com/bitcoin/bitcoin/pull/33067#discussion_r2568423936)
I feel there is scope for generalising this function further. Similar to how the previous class implementation used only the `CTransaction` class and the like.
Currently, this function is tied to the `MiniWallet` that I don't think is necessary. A more generic form of this function should be able to be used by the usual RPC wallets as well.
Consider the following diff that puts the responsibility of building the base parent transaction and submitting the final parent transaction to the caller
...
π¬ fanquake commented on issue "guix: re-enable exported symbol checking for RISC-V":
(https://github.com/bitcoin/bitcoin/issues/28095#issuecomment-3585628172)
While there hasn't been a response to our upstream issue: https://sourceware.org/bugzilla/show_bug.cgi?id=32783, the test case seems to be improved as of binutils 2.45. I can recreate the issue using `2.44`. i.e:
```bash
# riscv64-linux-gnu-ld --version
GNU ld (GNU Binutils for Debian) 2.44
# riscv64-linux-gnu-g++ test.cpp -o test.rv -Wl,--exclude-libs,ALL -fvisibility=hidden -static-libstdc++
# riscv64-linux-gnu-objdump -T test.rv|grep "\.text"
0000000000016820 l d .text 0000000000000000
...
(https://github.com/bitcoin/bitcoin/issues/28095#issuecomment-3585628172)
While there hasn't been a response to our upstream issue: https://sourceware.org/bugzilla/show_bug.cgi?id=32783, the test case seems to be improved as of binutils 2.45. I can recreate the issue using `2.44`. i.e:
```bash
# riscv64-linux-gnu-ld --version
GNU ld (GNU Binutils for Debian) 2.44
# riscv64-linux-gnu-g++ test.cpp -o test.rv -Wl,--exclude-libs,ALL -fvisibility=hidden -static-libstdc++
# riscv64-linux-gnu-objdump -T test.rv|grep "\.text"
0000000000016820 l d .text 0000000000000000
...
π¬ maflcko commented on pull request "log: Use more severe log level (warn/err) where appropriate":
(https://github.com/bitcoin/bitcoin/pull/33960#discussion_r2568464699)
hmm, probably should be an `Assume(false)`, but I only want to change the logs here, not the other behavior. It is dead code either way.
(https://github.com/bitcoin/bitcoin/pull/33960#discussion_r2568464699)
hmm, probably should be an `Assume(false)`, but I only want to change the logs here, not the other behavior. It is dead code either way.
π¬ maflcko commented on pull request "log: Use more severe log level (warn/err) where appropriate":
(https://github.com/bitcoin/bitcoin/pull/33960#discussion_r2568466473)
hmm, probably should be an `Assume(false)`, but I only want to change the logs here, not the other behavior. it is dead code either way.
(https://github.com/bitcoin/bitcoin/pull/33960#discussion_r2568466473)
hmm, probably should be an `Assume(false)`, but I only want to change the logs here, not the other behavior. it is dead code either way.
π¬ maflcko commented on pull request "log: Use more severe log level (warn/err) where appropriate":
(https://github.com/bitcoin/bitcoin/pull/33960#discussion_r2568466984)
I don't think the node shuts down here (fatal error). Also, this is dead code, so I don't think it matters much.
(https://github.com/bitcoin/bitcoin/pull/33960#discussion_r2568466984)
I don't think the node shuts down here (fatal error). Also, this is dead code, so I don't think it matters much.