💬 furszy commented on pull request "Add max_tx_weight to transaction funding options":
(https://github.com/bitcoin/bitcoin/pull/29264#discussion_r1465002873)
> > I haven't gone deeper over this PR yet but the message you are adding here doesn't seems like an error. It is just describing the "no solution found for the provided input set" transient situation.
>
> Are you saying it's not confusing to give insufficient funds message on failure to stay within the weight limits given? I guess that's fine for now?
Not really. I was only referring to this line in question. Which, in a first glance, isn't retrieving an exceeded weight limit error. Still
...
(https://github.com/bitcoin/bitcoin/pull/29264#discussion_r1465002873)
> > I haven't gone deeper over this PR yet but the message you are adding here doesn't seems like an error. It is just describing the "no solution found for the provided input set" transient situation.
>
> Are you saying it's not confusing to give insufficient funds message on failure to stay within the weight limits given? I guess that's fine for now?
Not really. I was only referring to this line in question. Which, in a first glance, isn't retrieving an exceeded weight limit error. Still
...
📝 vasild opened a pull request: "util: check for errors after close and read in AutoFile"
(https://github.com/bitcoin/bitcoin/pull/29307)
1. `fclose(3)` may fail to flush the previously written data to disk, thus a failing `fclose(3)` is as serious as a failing `fwrite(3)`. Previously the code ignored `fclose(3)` failures. This PR improves the explicit callers to check whether it failed. However there is a design issue that `fclose(3)` is also called from the `AutoFile` destructor. There is no good way to signal a failure to the caller from the destructor. Maybe one of:
1.1. `fflush(3)` after each write to the file (and throw i
...
(https://github.com/bitcoin/bitcoin/pull/29307)
1. `fclose(3)` may fail to flush the previously written data to disk, thus a failing `fclose(3)` is as serious as a failing `fwrite(3)`. Previously the code ignored `fclose(3)` failures. This PR improves the explicit callers to check whether it failed. However there is a design issue that `fclose(3)` is also called from the `AutoFile` destructor. There is no good way to signal a failure to the caller from the destructor. Maybe one of:
1.1. `fflush(3)` after each write to the file (and throw i
...
💬 vasild commented on pull request "Make (Read/Write)BinaryFile work with char vector, use AutoFile":
(https://github.com/bitcoin/bitcoin/pull/29229#discussion_r1465007760)
Logged as https://github.com/bitcoin/bitcoin/pull/29307
(https://github.com/bitcoin/bitcoin/pull/29229#discussion_r1465007760)
Logged as https://github.com/bitcoin/bitcoin/pull/29307
💬 vasild commented on pull request "Make (Read/Write)BinaryFile work with char vector, use AutoFile":
(https://github.com/bitcoin/bitcoin/pull/29229#discussion_r1465009114)
> if there is a bug in the current code in master, it should be fixed.
Done in https://github.com/bitcoin/bitcoin/pull/29307
(https://github.com/bitcoin/bitcoin/pull/29229#discussion_r1465009114)
> if there is a bug in the current code in master, it should be fixed.
Done in https://github.com/bitcoin/bitcoin/pull/29307
💬 ryanofsky commented on pull request "build: Enable -Wunreachable-code":
(https://github.com/bitcoin/bitcoin/pull/28999#discussion_r1465010916)
In commit "build: Enable -Wunreachable-code" (fa8adbe7c17b16cf7ecd16eb9f3f1792e9da2876)
Not a big deal, but IMO, this change is not an improvement. The unreachable code warning seems helpful to catch cases where code is intentionally unreachable. Or to encourage rewriting code where code is intentionally reachable but written in a hard to understand way.
But this code was intentionally unreachable in obvious way with two return statements right next to each other. It was written this way s
...
(https://github.com/bitcoin/bitcoin/pull/28999#discussion_r1465010916)
In commit "build: Enable -Wunreachable-code" (fa8adbe7c17b16cf7ecd16eb9f3f1792e9da2876)
Not a big deal, but IMO, this change is not an improvement. The unreachable code warning seems helpful to catch cases where code is intentionally unreachable. Or to encourage rewriting code where code is intentionally reachable but written in a hard to understand way.
But this code was intentionally unreachable in obvious way with two return statements right next to each other. It was written this way s
...
💬 glozow commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1465019439)
fwiw I've opened #29306 which implements the alternative approach. Again I'm trying to keep the scope of this PR to the 1p1c topology restriction, and we can consider additional features like sibling eviction independently.
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1465019439)
fwiw I've opened #29306 which implements the alternative approach. Again I'm trying to keep the scope of this PR to the 1p1c topology restriction, and we can consider additional features like sibling eviction independently.
💬 murchandamus commented on pull request "Add max_tx_weight to transaction funding options":
(https://github.com/bitcoin/bitcoin/pull/29264#discussion_r1465052341)
Mh, it’s a bit of a smell if we cannot explain this.
I considered input counter, empty witness stack, that we might estimate for a high-R signature in a PSBT, because we didn’t sign it, and that we sometimes get a lower `r` value in the signature and therefore one of the two values is a byte shorter. But our default address type would be at least wrapped segwit, so that would only explain a weight difference of 1. I don’t have a good idea at the moment. @achow101: Do you perhaps have an idea wh
...
(https://github.com/bitcoin/bitcoin/pull/29264#discussion_r1465052341)
Mh, it’s a bit of a smell if we cannot explain this.
I considered input counter, empty witness stack, that we might estimate for a high-R signature in a PSBT, because we didn’t sign it, and that we sometimes get a lower `r` value in the signature and therefore one of the two values is a byte shorter. But our default address type would be at least wrapped segwit, so that would only explain a weight difference of 1. I don’t have a good idea at the moment. @achow101: Do you perhaps have an idea wh
...
💬 fanquake commented on pull request "[WIP] C++20 std::views::reverse":
(https://github.com/bitcoin/bitcoin/pull/28687#issuecomment-1908318131)
> (Possibly?) fixed in https://github.com/bitcoin/bitcoin/pull/29275
Looks like it does, this compiles for me locally atleast: https://github.com/fanquake/bitcoin/tree/28687_29275.
@stickies-v can you rebase this branch on top of #29275.
(https://github.com/bitcoin/bitcoin/pull/28687#issuecomment-1908318131)
> (Possibly?) fixed in https://github.com/bitcoin/bitcoin/pull/29275
Looks like it does, this compiles for me locally atleast: https://github.com/fanquake/bitcoin/tree/28687_29275.
@stickies-v can you rebase this branch on top of #29275.
📝 dergoegge converted_to_draft a pull request: "fuzz: Use FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION for pow checks"
(https://github.com/bitcoin/bitcoin/pull/29305)
Alternative to the mocking of `CheckProofOfWork` in #28043 for avoiding fuzzers to be blocked on proof-of-work checks.
More on `FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION`: https://llvm.org/docs/LibFuzzer.html#fuzzer-friendly-build-mode
(https://github.com/bitcoin/bitcoin/pull/29305)
Alternative to the mocking of `CheckProofOfWork` in #28043 for avoiding fuzzers to be blocked on proof-of-work checks.
More on `FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION`: https://llvm.org/docs/LibFuzzer.html#fuzzer-friendly-build-mode
💬 dergoegge commented on pull request "fuzz: Use FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION for pow checks":
(https://github.com/bitcoin/bitcoin/pull/29305#issuecomment-1908321342)
Using clang with `-fsanitize=fuzzer` does actually not define this by default.
(https://github.com/bitcoin/bitcoin/pull/29305#issuecomment-1908321342)
Using clang with `-fsanitize=fuzzer` does actually not define this by default.
💬 instagibbs commented on pull request "Mempool util: Add RBF diagram checks for single chunks against clusters of size 2":
(https://github.com/bitcoin/bitcoin/pull/29242#issuecomment-1908321622)
@dergoegge fuzzer created a circular reference which caused incongruities between descendant counts and actually walking the descendants in mempool. Please run again?
(https://github.com/bitcoin/bitcoin/pull/29242#issuecomment-1908321622)
@dergoegge fuzzer created a circular reference which caused incongruities between descendant counts and actually walking the descendants in mempool. Please run again?
💬 fanquake commented on pull request "depends: Do not override `CFLAGS` when building SQLite with `DEBUG=1`":
(https://github.com/bitcoin/bitcoin/pull/29287#issuecomment-1908327313)
> Changes in https://github.com/bitcoin/bitcoin/pull/29282 might not be strictly required now. However, I consider them an improvement.
I think it'd be easiest to make (any) additional changes together here, so we don't have to go over the flags twice. Can you combine any relevant changes and close the other PR.
> Could this be the root cause for https://github.com/bitcoin/bitcoin/issues/27222?
I think so. Rebased #27495 on top.
Related, I guess `-g` will no-longer be used, when comp
...
(https://github.com/bitcoin/bitcoin/pull/29287#issuecomment-1908327313)
> Changes in https://github.com/bitcoin/bitcoin/pull/29282 might not be strictly required now. However, I consider them an improvement.
I think it'd be easiest to make (any) additional changes together here, so we don't have to go over the flags twice. Can you combine any relevant changes and close the other PR.
> Could this be the root cause for https://github.com/bitcoin/bitcoin/issues/27222?
I think so. Rebased #27495 on top.
Related, I guess `-g` will no-longer be used, when comp
...
💬 fanquake commented on pull request "depends: patch libool out of libnatpmp/miniupnpc":
(https://github.com/bitcoin/bitcoin/pull/29298#issuecomment-1908331771)
Concept ACK - maybe upstream will take these patches as well? I guess we can try and send them, given we'll have to send CMake fixes in either case.
Rebased #21778 on top of this rather than #29232.
(https://github.com/bitcoin/bitcoin/pull/29298#issuecomment-1908331771)
Concept ACK - maybe upstream will take these patches as well? I guess we can try and send them, given we'll have to send CMake fixes in either case.
Rebased #21778 on top of this rather than #29232.
🚀 fanquake merged a pull request: "fuzz: Exit and log stderr for parse_test_list errors"
(https://github.com/bitcoin/bitcoin/pull/29304)
(https://github.com/bitcoin/bitcoin/pull/29304)
🤔 murchandamus reviewed a pull request: "wallet: Keep track of the wallet's own transaction outputs in memory"
(https://github.com/bitcoin/bitcoin/pull/27286#pullrequestreview-1841667091)
ReACK https://github.com/bitcoin/bitcoin/commit/39f0ab3e126704b30f7ed2b86690f60b631d2ec4 compared to my prior review, there are only the changes mentioned in response to @aureleoules.
(https://github.com/bitcoin/bitcoin/pull/27286#pullrequestreview-1841667091)
ReACK https://github.com/bitcoin/bitcoin/commit/39f0ab3e126704b30f7ed2b86690f60b631d2ec4 compared to my prior review, there are only the changes mentioned in response to @aureleoules.
💬 fanquake commented on pull request "serialization: c++20 endian/byteswap/clz modernization":
(https://github.com/bitcoin/bitcoin/pull/29263#issuecomment-1908366535)
Concept ACK - I think moving to using the builtins is ok. With the eventual plan to migrate to `std::byteswap`.
@aureleoules everytime I visit the [coverage/benchmarks for this PR](https://corecheck.dev/bitcoin/bitcoin/pulls/29263), I get a 500 error. Can you take a look?
(https://github.com/bitcoin/bitcoin/pull/29263#issuecomment-1908366535)
Concept ACK - I think moving to using the builtins is ok. With the eventual plan to migrate to `std::byteswap`.
@aureleoules everytime I visit the [coverage/benchmarks for this PR](https://corecheck.dev/bitcoin/bitcoin/pulls/29263), I get a 500 error. Can you take a look?
📝 ismaelsadeeq opened a pull request: "doc: clarify that `BroadcastTransaction` comment"
(https://github.com/bitcoin/bitcoin/pull/29308)
`BroadcastTransaction` is also called by `submitpackage` RPC.
The comment needs to be updated, all transactions that are accepted into the mempool post package processing are broadcasted to peers individually here
https://github.com/bitcoin/bitcoin/blob/ea4ddd8652d9dd1e7698e2a6f84c606cf24a2e3e/src/rpc/mempool.cpp#L926
(https://github.com/bitcoin/bitcoin/pull/29308)
`BroadcastTransaction` is also called by `submitpackage` RPC.
The comment needs to be updated, all transactions that are accepted into the mempool post package processing are broadcasted to peers individually here
https://github.com/bitcoin/bitcoin/blob/ea4ddd8652d9dd1e7698e2a6f84c606cf24a2e3e/src/rpc/mempool.cpp#L926
👍 vasild approved a pull request: "doc: clarify `BroadcastTransaction` comment"
(https://github.com/bitcoin/bitcoin/pull/29308#pullrequestreview-1841734543)
ACK cca20d94b787d0881a61509445f4827f913e051d
I was looking at the same comment and was thinking it needs an update. Thanks!
(https://github.com/bitcoin/bitcoin/pull/29308#pullrequestreview-1841734543)
ACK cca20d94b787d0881a61509445f4827f913e051d
I was looking at the same comment and was thinking it needs an update. Thanks!
💬 theuni commented on pull request "build: always set `-g -O2` in `CORE_CXXFLAGS`":
(https://github.com/bitcoin/bitcoin/pull/29205#issuecomment-1908411460)
> > While always overriding Autotools' defaults, are there any particular reasons not using -O3 instead of -O2?
>
> AFAIK `-O3` isn't safe.
I remember these discussions almost 10 years ago when compilers (notably gcc) put aggressive and (iirc?) non-standards-compliant optimizations behind `-O3`. I'm guessing that's probably not the case anymore these days, but I do agree somewhat with @luke-jr that we'd need to investigate the safety of this before blindly changing.
Either way, it's out
...
(https://github.com/bitcoin/bitcoin/pull/29205#issuecomment-1908411460)
> > While always overriding Autotools' defaults, are there any particular reasons not using -O3 instead of -O2?
>
> AFAIK `-O3` isn't safe.
I remember these discussions almost 10 years ago when compilers (notably gcc) put aggressive and (iirc?) non-standards-compliant optimizations behind `-O3`. I'm guessing that's probably not the case anymore these days, but I do agree somewhat with @luke-jr that we'd need to investigate the safety of this before blindly changing.
Either way, it's out
...
💬 TheCharlatan commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1465145033)
Good catch!
I think the real problem here is though that the scheduler is now created too early, so I'll revert moving its initialization out of `AppInitMain` and will instead leave it there and initialize the signals right after it. Then we can have all these components be initialized in `ChainTestingSetup`.
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1465145033)
Good catch!
I think the real problem here is though that the scheduler is now created too early, so I'll revert moving its initialization out of `AppInitMain` and will instead leave it there and initialize the signals right after it. Then we can have all these components be initialized in `ChainTestingSetup`.