💬 furszy commented on pull request "Add max_tx_weight to transaction funding options":
(https://github.com/bitcoin/bitcoin/pull/29264#discussion_r1464899774)
The empty `util::Error()` is used to retrieve a "no solution found for the provided inputs set". If you specify an error message, then the wallet coin selection procedure treats it as a real error and bubble it up when no other selection algorithm succeeded.
The goal of this distinction is to differentiate between errors that should be bubble up and coin selection algorithms partial solutions. Remember that we are running each coin selection algorithm up to 7 times. Extending the inputs set o
...
(https://github.com/bitcoin/bitcoin/pull/29264#discussion_r1464899774)
The empty `util::Error()` is used to retrieve a "no solution found for the provided inputs set". If you specify an error message, then the wallet coin selection procedure treats it as a real error and bubble it up when no other selection algorithm succeeded.
The goal of this distinction is to differentiate between errors that should be bubble up and coin selection algorithms partial solutions. Remember that we are running each coin selection algorithm up to 7 times. Extending the inputs set o
...
💬 fanquake commented on pull request "Add max_tx_weight to transaction funding options":
(https://github.com/bitcoin/bitcoin/pull/29264#discussion_r1464933701)
> The empty util::Error() is used to retrieve a "no solution found for the provided inputs set". If you specify an error message, then the wallet coin selection procedure treats it as a real error and bubble it up when no other selection algorithm succeeded.
Ok. So then this is just a (confusing) misuse of the `Util::` API, where the wallet code is shoehorning non-error handling into an error handler.
(https://github.com/bitcoin/bitcoin/pull/29264#discussion_r1464933701)
> The empty util::Error() is used to retrieve a "no solution found for the provided inputs set". If you specify an error message, then the wallet coin selection procedure treats it as a real error and bubble it up when no other selection algorithm succeeded.
Ok. So then this is just a (confusing) misuse of the `Util::` API, where the wallet code is shoehorning non-error handling into an error handler.
💬 ryanofsky commented on pull request "validation: improve checkblockindex comments":
(https://github.com/bitcoin/bitcoin/pull/29299#discussion_r1464948949)
> nit: Missing nTx check, according to comment?
IMO, the suggestion to add `pindex->nTx >=1` does not make the code clearer, and logically adding it should not have any effect because if `nTx` is 0 (`nTx` is unsigned), that's just the previous "Transaction may be completely unset case" and this comparison will never be executed.
(https://github.com/bitcoin/bitcoin/pull/29299#discussion_r1464948949)
> nit: Missing nTx check, according to comment?
IMO, the suggestion to add `pindex->nTx >=1` does not make the code clearer, and logically adding it should not have any effect because if `nTx` is 0 (`nTx` is unsigned), that's just the previous "Transaction may be completely unset case" and this comparison will never be executed.
🤔 stickies-v reviewed a pull request: "Nuke adjusted time from validation (attempt 2)"
(https://github.com/bitcoin/bitcoin/pull/28956#pullrequestreview-1838562579)
Approach ACK
I think keeping this diff small and focusing on just the (non-warning) functional behaviour change is the best approach to reduce friction to get more high quality feedback on whether this is indeed a safety improvement (I lean towards yes). Code LGTM ff9039f6ea876bab2c40a06a93e0dd087f445fa2, but I think this warrants a release note since we are changing some security assumptions, making users aware of that seems like the way to go imo.
---
nit: latest force push regressed
...
(https://github.com/bitcoin/bitcoin/pull/28956#pullrequestreview-1838562579)
Approach ACK
I think keeping this diff small and focusing on just the (non-warning) functional behaviour change is the best approach to reduce friction to get more high quality feedback on whether this is indeed a safety improvement (I lean towards yes). Code LGTM ff9039f6ea876bab2c40a06a93e0dd087f445fa2, but I think this warrants a release note since we are changing some security assumptions, making users aware of that seems like the way to go imo.
---
nit: latest force push regressed
...
💬 stickies-v commented on pull request "Nuke adjusted time from validation (attempt 2)":
(https://github.com/bitcoin/bitcoin/pull/28956#discussion_r1464827885)
This latest force-push seems to have accidentally reverted the necessary doc change in `chain.h`:
<details>
<summary>git diff on ff9039f6ea</summary>
```diff
diff --git a/src/chain.h b/src/chain.h
index f9121fb861..ab58fad5b9 100644
--- a/src/chain.h
+++ b/src/chain.h
@@ -19,7 +19,7 @@
/**
* Maximum amount of time that a block timestamp is allowed to exceed the
- * current network-adjusted time before the block will be accepted.
+ * current time before the block will be acc
...
(https://github.com/bitcoin/bitcoin/pull/28956#discussion_r1464827885)
This latest force-push seems to have accidentally reverted the necessary doc change in `chain.h`:
<details>
<summary>git diff on ff9039f6ea</summary>
```diff
diff --git a/src/chain.h b/src/chain.h
index f9121fb861..ab58fad5b9 100644
--- a/src/chain.h
+++ b/src/chain.h
@@ -19,7 +19,7 @@
/**
* Maximum amount of time that a block timestamp is allowed to exceed the
- * current network-adjusted time before the block will be accepted.
+ * current time before the block will be acc
...
💬 sipa commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1464954826)
TCP/IP presents a stream interface that sends/receives bytes. You can only receive multiples of bytes.
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1464954826)
TCP/IP presents a stream interface that sends/receives bytes. You can only receive multiples of bytes.
💬 furszy commented on pull request "Add max_tx_weight to transaction funding options":
(https://github.com/bitcoin/bitcoin/pull/29264#discussion_r1464960277)
> > The empty util::Error() is used to retrieve a "no solution found for the provided inputs set". If you specify an error message, then the wallet coin selection procedure treats it as a real error and bubble it up when no other selection algorithm succeeded.
>
> Ok. So then this is just a (confusing) misuse of the `Util::` API, where the wallet code is shoehorning non-error handling into an error handler.
Ideally, we should retrieve more info within the error return path and not just the
...
(https://github.com/bitcoin/bitcoin/pull/29264#discussion_r1464960277)
> > The empty util::Error() is used to retrieve a "no solution found for the provided inputs set". If you specify an error message, then the wallet coin selection procedure treats it as a real error and bubble it up when no other selection algorithm succeeded.
>
> Ok. So then this is just a (confusing) misuse of the `Util::` API, where the wallet code is shoehorning non-error handling into an error handler.
Ideally, we should retrieve more info within the error return path and not just the
...
💬 stickies-v commented on pull request "rpc: improve submitpackage documentation and other improvements":
(https://github.com/bitcoin/bitcoin/pull/29292#discussion_r1464963171)
> Seems we are missing test coverage for this. Maybe add one to rpc_packages.py?
I'll have a look!
> Also, why delete the empty vector case? That should throw a JSONRPCError too, no?
We [throw](https://github.com/bitcoin/bitcoin/pull/29292/files#diff-9c5b83de6dc84af277e352c88b9291aa44340a3c75f572a0b51661eb0a838de9R879-R880) a `JSONRPCError` here if package size < 2, so all this should do is remove the confusing error message that the array needs to be at least size 1 (when actually it's
...
(https://github.com/bitcoin/bitcoin/pull/29292#discussion_r1464963171)
> Seems we are missing test coverage for this. Maybe add one to rpc_packages.py?
I'll have a look!
> Also, why delete the empty vector case? That should throw a JSONRPCError too, no?
We [throw](https://github.com/bitcoin/bitcoin/pull/29292/files#diff-9c5b83de6dc84af277e352c88b9291aa44340a3c75f572a0b51661eb0a838de9R879-R880) a `JSONRPCError` here if package size < 2, so all this should do is remove the confusing error message that the array needs to be at least size 1 (when actually it's
...
💬 instagibbs commented on pull request "Add max_tx_weight to transaction funding options":
(https://github.com/bitcoin/bitcoin/pull/29264#discussion_r1464970743)
> 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?
(https://github.com/bitcoin/bitcoin/pull/29264#discussion_r1464970743)
> 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?
📝 dergoegge opened 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: Test headers pre-sync through p2p interface":
(https://github.com/bitcoin/bitcoin/pull/28043#discussion_r1464982811)
Opened https://github.com/bitcoin/bitcoin/pull/29305 to discuss, I think it actually makes more sense than the mocking
(https://github.com/bitcoin/bitcoin/pull/28043#discussion_r1464982811)
Opened https://github.com/bitcoin/bitcoin/pull/29305 to discuss, I think it actually makes more sense than the mocking
📝 glozow opened a pull request: "policy: enable sibling eviction for v3 transactions"
(https://github.com/bitcoin/bitcoin/pull/29306)
Built on top of #28948 as a demonstration/proposal of another nice feature you can do when you only need to deal with 1p1c packages.
When we receive a v3 transaction that would bust a mempool transaction's descendant limit, instead of rejecting the new tx, consider replacing the other descendant if it is much higher feerate (using existing RBF criteria to assess that it's more incentive compatible and to avoid DoS).
Delving post with more background and motivation: https://delvingbitcoin.o
...
(https://github.com/bitcoin/bitcoin/pull/29306)
Built on top of #28948 as a demonstration/proposal of another nice feature you can do when you only need to deal with 1p1c packages.
When we receive a v3 transaction that would bust a mempool transaction's descendant limit, instead of rejecting the new tx, consider replacing the other descendant if it is much higher feerate (using existing RBF criteria to assess that it's more incentive compatible and to avoid DoS).
Delving post with more background and motivation: https://delvingbitcoin.o
...
💬 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.