💬 instagibbs commented on pull request "Add max_tx_weight to transaction funding options":
(https://github.com/bitcoin/bitcoin/pull/29264#discussion_r1464835996)
if this is a bit of overcounting, that's probably ok?
(https://github.com/bitcoin/bitcoin/pull/29264#discussion_r1464835996)
if this is a bit of overcounting, that's probably ok?
💬 fanquake commented on pull request "Add max_tx_weight to transaction funding options":
(https://github.com/bitcoin/bitcoin/pull/29264#discussion_r1464840653)
Is the breakage not an actual issue: https://cirrus-ci.com/task/6124189116006400?logs=ci#L3274
```bash
test_framework.authproxy.JSONRPCException: Only 4999872180 coins to cover target value of 5100000840 (-4)
```
(https://github.com/bitcoin/bitcoin/pull/29264#discussion_r1464840653)
Is the breakage not an actual issue: https://cirrus-ci.com/task/6124189116006400?logs=ci#L3274
```bash
test_framework.authproxy.JSONRPCException: Only 4999872180 coins to cover target value of 5100000840 (-4)
```
💬 instagibbs commented on pull request "Add max_tx_weight to transaction funding options":
(https://github.com/bitcoin/bitcoin/pull/29264#discussion_r1464842951)
I can turn the error on and off locally by adding/removing the string from the error.
(https://github.com/bitcoin/bitcoin/pull/29264#discussion_r1464842951)
I can turn the error on and off locally by adding/removing the string from the error.
💬 TheCharlatan commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1464847173)
This is on purpose (note the lower case `t`) to fix a documentation typo. I could have added a regex for doing it in one line, but that seems even less readable.
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1464847173)
This is on purpose (note the lower case `t`) to fix a documentation typo. I could have added a regex for doing it in one line, but that seems even less readable.
💬 0xB10C commented on pull request "tracing: Only prepare tracepoint arguments when actually tracing":
(https://github.com/bitcoin/bitcoin/pull/26593#issuecomment-1908048127)
Rebased to include https://github.com/bitcoin/bitcoin/pull/29272
(https://github.com/bitcoin/bitcoin/pull/26593#issuecomment-1908048127)
Rebased to include https://github.com/bitcoin/bitcoin/pull/29272
💬 fanquake commented on pull request "Add max_tx_weight to transaction funding options":
(https://github.com/bitcoin/bitcoin/pull/29264#discussion_r1464860895)
The wallet code seems to be the only place where this pattern of `util::Error()` is used. Everywhere actual error strings are always provided.
(https://github.com/bitcoin/bitcoin/pull/29264#discussion_r1464860895)
The wallet code seems to be the only place where this pattern of `util::Error()` is used. Everywhere actual error strings are always provided.
💬 Ahmatmufid commented on pull request "doc: FreeBSD build doc updates to reflect removal of install_db4.sh":
(https://github.com/bitcoin/bitcoin/pull/26773#issuecomment-1908096016)
Oke"👍👍👍👍
(https://github.com/bitcoin/bitcoin/pull/26773#issuecomment-1908096016)
Oke"👍👍👍👍
💬 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
...