💬 glozow commented on pull request "rpc: improve submitpackage documentation and other improvements":
(https://github.com/bitcoin/bitcoin/pull/29292#discussion_r1464807756)
Also, why delete the empty vector case? That should throw a JSONRPCError too, no?
(https://github.com/bitcoin/bitcoin/pull/29292#discussion_r1464807756)
Also, why delete the empty vector case? That should throw a JSONRPCError too, no?
💬 fjahr commented on pull request "contrib: add test for bucketing with asmap":
(https://github.com/bitcoin/bitcoin/pull/28869#issuecomment-1907998137)
Code looks good. I have been testing this with the `asmap.dat` file from https://github.com/fjahr/asmap-data/pull/6 and I noticed that after adding 60-70 entries the program slows down a lot and sometimes takes over a minute to add the next address. Have you seen this as well and do you know why this is @brunoerg ?
(https://github.com/bitcoin/bitcoin/pull/28869#issuecomment-1907998137)
Code looks good. I have been testing this with the `asmap.dat` file from https://github.com/fjahr/asmap-data/pull/6 and I noticed that after adding 60-70 entries the program slows down a lot and sometimes takes over a minute to add the next address. Have you seen this as well and do you know why this is @brunoerg ?
💬 vasild commented on pull request "Make (Read/Write)BinaryFile work with char vector, use AutoFile":
(https://github.com/bitcoin/bitcoin/pull/29229#discussion_r1464821741)
I would extend the check to:
```diff
void AutoFile::read(Span<std::byte> dst)
{
- if (detail_fread(dst) != dst.size()) {
+ if (detail_fread(dst) != dst.size() || std::ferror(m_file)) {
throw std::ios_base::failure(feof() ? "AutoFile::read: end of file" : "AutoFile::read: fread failed");
```
(https://github.com/bitcoin/bitcoin/pull/29229#discussion_r1464821741)
I would extend the check to:
```diff
void AutoFile::read(Span<std::byte> dst)
{
- if (detail_fread(dst) != dst.size()) {
+ if (detail_fread(dst) != dst.size() || std::ferror(m_file)) {
throw std::ios_base::failure(feof() ? "AutoFile::read: end of file" : "AutoFile::read: fread failed");
```
💬 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