💬 maflcko commented on pull request "Fees: add Fee rate Forecaster Manager":
(https://github.com/bitcoin/bitcoin/pull/31664#discussion_r2099526827)
could remove the redundant comments, given that at least one of them is wrong?
(https://github.com/bitcoin/bitcoin/pull/31664#discussion_r2099526827)
could remove the redundant comments, given that at least one of them is wrong?
💬 maflcko commented on pull request "ci, iwyu: Treat warnings as errors for specific targets":
(https://github.com/bitcoin/bitcoin/pull/31308#issuecomment-2896845391)
concept ack, but yeah, needs rebase
(https://github.com/bitcoin/bitcoin/pull/31308#issuecomment-2896845391)
concept ack, but yeah, needs rebase
💬 Sjors commented on pull request "wallet: `addhdkey` RPC to add just keys to wallets via new `unused(KEY)` descriptor":
(https://github.com/bitcoin/bitcoin/pull/29136#issuecomment-2896961033)
utACK b23cefb4f3abe33c7bc933b60f1c0d15137c627c
(https://github.com/bitcoin/bitcoin/pull/29136#issuecomment-2896961033)
utACK b23cefb4f3abe33c7bc933b60f1c0d15137c627c
💬 Sjors commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2099616827)
Sounds fine, forgot to push this?
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2099616827)
Sounds fine, forgot to push this?
💬 maflcko commented on pull request "fs: remove `_POSIX_C_SOURCE` defining":
(https://github.com/bitcoin/bitcoin/pull/32460#issuecomment-2896998818)
> I wonder if there is a platform without `posix_fallocate` (outside of macos and windows). If not, the fallback can probably be fully removed, along with the detection and it can just be assumed to be always present?
I am asking, because the fallback was just the normal code in the beginning (bba89aa82a80f0373dcb7288d96d5b0fcb453d73) and was not removed in 288fdc092aff9d7e0cea159196b2e96044a786c7.
However, even bioniC has `posix_fallocate` since API level 21 (https://android.googlesource
...
(https://github.com/bitcoin/bitcoin/pull/32460#issuecomment-2896998818)
> I wonder if there is a platform without `posix_fallocate` (outside of macos and windows). If not, the fallback can probably be fully removed, along with the detection and it can just be assumed to be always present?
I am asking, because the fallback was just the normal code in the beginning (bba89aa82a80f0373dcb7288d96d5b0fcb453d73) and was not removed in 288fdc092aff9d7e0cea159196b2e96044a786c7.
However, even bioniC has `posix_fallocate` since API level 21 (https://android.googlesource
...
💬 maflcko commented on pull request "fs: remove `_POSIX_C_SOURCE` defining":
(https://github.com/bitcoin/bitcoin/pull/32460#issuecomment-2897032758)
> And the other slimmer libcs may not have `ftruncate` (https://www.sourceware.org/newlib/libc.html), so they can't be used anyway.
Actually, it is used in https://github.com/bitcoin/bitcoin/pull/31425, so I wonder how that compiles and links. I guess it will start failing once more than a trivial dummy code is linked.
(https://github.com/bitcoin/bitcoin/pull/32460#issuecomment-2897032758)
> And the other slimmer libcs may not have `ftruncate` (https://www.sourceware.org/newlib/libc.html), so they can't be used anyway.
Actually, it is used in https://github.com/bitcoin/bitcoin/pull/31425, so I wonder how that compiles and links. I guess it will start failing once more than a trivial dummy code is linked.
💬 vasild commented on pull request "tests: Expand HTTP coverage to assert libevent behavior":
(https://github.com/bitcoin/bitcoin/pull/32408#discussion_r2099675521)
Did you try to start the timer before connecting? Like this:
```diff
# Get the underlying socket from HTTP connection so we can send something unusual
conn = http.client.HTTPConnection(urlNode2.hostname, urlNode2.port)
+ start = time.time()
conn.connect()
sock = conn.sock
sock.sendall(http_request.encode("utf-8"))
# Wait for response, but expect a timeout disconnection after 1 second
- start = time.time()
re
...
(https://github.com/bitcoin/bitcoin/pull/32408#discussion_r2099675521)
Did you try to start the timer before connecting? Like this:
```diff
# Get the underlying socket from HTTP connection so we can send something unusual
conn = http.client.HTTPConnection(urlNode2.hostname, urlNode2.port)
+ start = time.time()
conn.connect()
sock = conn.sock
sock.sendall(http_request.encode("utf-8"))
# Wait for response, but expect a timeout disconnection after 1 second
- start = time.time()
re
...
💬 vasild commented on pull request "tests: Expand HTTP coverage to assert libevent behavior":
(https://github.com/bitcoin/bitcoin/pull/32408#discussion_r2099677203)
_continued the discussion in https://github.com/bitcoin/bitcoin/pull/32408#discussion_r2081584805_
(https://github.com/bitcoin/bitcoin/pull/32408#discussion_r2099677203)
_continued the discussion in https://github.com/bitcoin/bitcoin/pull/32408#discussion_r2081584805_
💬 maflcko commented on pull request "RFC: Riscv bare metal CI job":
(https://github.com/bitcoin/bitcoin/pull/31425#issuecomment-2897104226)
Coming from https://github.com/bitcoin/bitcoin/pull/32460#issuecomment-2897032758, I noticed that https://www.sourceware.org/newlib/libc.html doesn't list `ftruncate`, so I guess the build will fail once more code is compiled/linked?
(https://github.com/bitcoin/bitcoin/pull/31425#issuecomment-2897104226)
Coming from https://github.com/bitcoin/bitcoin/pull/32460#issuecomment-2897032758, I noticed that https://www.sourceware.org/newlib/libc.html doesn't list `ftruncate`, so I guess the build will fail once more code is compiled/linked?
💬 fanquake commented on pull request "RFC: Riscv bare metal CI job":
(https://github.com/bitcoin/bitcoin/pull/31425#issuecomment-2897113177)
I think the docs might be incomplete / outdated? See https://github.com/bminor/newlib/blob/b39b510c1ce68757e79410585262ca2cd48da839/newlib/libc/include/sys/unistd.h#L287.
(https://github.com/bitcoin/bitcoin/pull/31425#issuecomment-2897113177)
I think the docs might be incomplete / outdated? See https://github.com/bminor/newlib/blob/b39b510c1ce68757e79410585262ca2cd48da839/newlib/libc/include/sys/unistd.h#L287.
💬 maflcko commented on pull request "rpc: Support v3 raw transactions creation":
(https://github.com/bitcoin/bitcoin/pull/31936#issuecomment-2897127443)
> > `createrawtransaction` doesn't do other policy checks either, such as OP_RETURN checks, so one could argue this is fine as well.
>
> `fundrawtransaction` does though, and that was my point.
I don't think `fundrawtransaction` checks for OP_RETURN. At least locally, the following passes for me:
```
createrawtransaction '[]' '[{"data":"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
...
(https://github.com/bitcoin/bitcoin/pull/31936#issuecomment-2897127443)
> > `createrawtransaction` doesn't do other policy checks either, such as OP_RETURN checks, so one could argue this is fine as well.
>
> `fundrawtransaction` does though, and that was my point.
I don't think `fundrawtransaction` checks for OP_RETURN. At least locally, the following passes for me:
```
createrawtransaction '[]' '[{"data":"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
...
👍 vasild approved a pull request: "multiprocess: Add bitcoin wrapper executable"
(https://github.com/bitcoin/bitcoin/pull/31375#pullrequestreview-2856867196)
ACK a5ac43d98d1ad3ebed934f2c50208a85aae17e5e
(https://github.com/bitcoin/bitcoin/pull/31375#pullrequestreview-2856867196)
ACK a5ac43d98d1ad3ebed934f2c50208a85aae17e5e
💬 vasild commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2099737369)
This can be written like:
```cpp
util::ExecVp(exec_args[0], (char*const*)exec_args.data()); // If it returns then an error has occurred.
if (allow_notfound && errno == ENOENT) return false;
throw std::system_error(errno, std::system_category(), strprintf("execvp failed to execute '%s'", exec_args[0]));
```
it is less indentation and less code - no need for the "this will never be reached" `throw`.
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2099737369)
This can be written like:
```cpp
util::ExecVp(exec_args[0], (char*const*)exec_args.data()); // If it returns then an error has occurred.
if (allow_notfound && errno == ENOENT) return false;
throw std::system_error(errno, std::system_category(), strprintf("execvp failed to execute '%s'", exec_args[0]));
```
it is less indentation and less code - no need for the "this will never be reached" `throw`.
💬 vasild commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2099742506)
super nit, but I can't resist :rescue_worker_helmet: :
"null terminated strings" and "null terminated" array -- it is not the same "null":
```suggestion
//! array should consist of '\0'-terminated strings and be nullptr-terminated
//! itself, like the POSIX function.
```
:runner:
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2099742506)
super nit, but I can't resist :rescue_worker_helmet: :
"null terminated strings" and "null terminated" array -- it is not the same "null":
```suggestion
//! array should consist of '\0'-terminated strings and be nullptr-terminated
//! itself, like the POSIX function.
```
:runner:
💬 vasild commented on pull request "rpc: provide per message stats for global traffic via new RPC 'getnetmsgstats'":
(https://github.com/bitcoin/bitcoin/pull/29418#issuecomment-2897167995)
`626cc06c69...528b4d6b9c`: rebase and do https://github.com/bitcoin/bitcoin/pull/29418#discussion_r2098827990
(https://github.com/bitcoin/bitcoin/pull/29418#issuecomment-2897167995)
`626cc06c69...528b4d6b9c`: rebase and do https://github.com/bitcoin/bitcoin/pull/29418#discussion_r2098827990
🚀 fanquake merged a pull request: "psbt: add non-default sighash types to PSBTs and unify sighash type match checking"
(https://github.com/bitcoin/bitcoin/pull/31622)
(https://github.com/bitcoin/bitcoin/pull/31622)
💬 fanquake commented on pull request "random: Use modern Windows randomness functions":
(https://github.com/bitcoin/bitcoin/pull/32400#issuecomment-2897179032)
> But, it probably would still make sense to close https://github.com/bitcoin/bitcoin/issues/32391.
Yea, on our side, this should be enough to close #32391.
(https://github.com/bitcoin/bitcoin/pull/32400#issuecomment-2897179032)
> But, it probably would still make sense to close https://github.com/bitcoin/bitcoin/issues/32391.
Yea, on our side, this should be enough to close #32391.
💬 fanquake commented on pull request "build: Use clang-cl to build on Windows natively":
(https://github.com/bitcoin/bitcoin/pull/31507#issuecomment-2897204778)
Reading the description, I'm still not really clear on the goals here. It seems like this is just adding even more ways to compile for Windows, which we have to add more CI / and more work arounds for, but doesn't actually improve the Windows binaries we are shipping to end users in any way. This might make more sense if we were actually going to switch to shipping Windows binaries using Clang.
(https://github.com/bitcoin/bitcoin/pull/31507#issuecomment-2897204778)
Reading the description, I'm still not really clear on the goals here. It seems like this is just adding even more ways to compile for Windows, which we have to add more CI / and more work arounds for, but doesn't actually improve the Windows binaries we are shipping to end users in any way. This might make more sense if we were actually going to switch to shipping Windows binaries using Clang.
🚀 fanquake merged a pull request: "qt: drop unused watch-only functionality"
(https://github.com/bitcoin/bitcoin/pull/32459)
(https://github.com/bitcoin/bitcoin/pull/32459)
✅ maflcko closed an issue: "Dropping unused legacy wallet code"
(https://github.com/bitcoin-core/gui/issues/873)
(https://github.com/bitcoin-core/gui/issues/873)