💬 ajtowns commented on pull request "Early logging improvements":
(https://github.com/bitcoin/bitcoin/pull/30386#discussion_r1687409670)
The memusage is incremented later when `str_prefixed` is included in `buf`, so afaics this should already be accurate.
(https://github.com/bitcoin/bitcoin/pull/30386#discussion_r1687409670)
The memusage is incremented later when `str_prefixed` is included in `buf`, so afaics this should already be accurate.
💬 maflcko commented on issue "Fuzzing related configuration/build options can be improved":
(https://github.com/bitcoin/bitcoin/issues/30318#issuecomment-2244366554)
> Which are those flags?
`DABORT_ON_FAILED_ASSUME` right now, but there can be more in the future. See for example https://duckduckgo.com/?q=FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION . So having a build system flag to enable all of them is useful. Otherwise, if more fuzz-recommended build flags are added in the future, it will be a global breaking change again.
(https://github.com/bitcoin/bitcoin/issues/30318#issuecomment-2244366554)
> Which are those flags?
`DABORT_ON_FAILED_ASSUME` right now, but there can be more in the future. See for example https://duckduckgo.com/?q=FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION . So having a build system flag to enable all of them is useful. Otherwise, if more fuzz-recommended build flags are added in the future, it will be a global breaking change again.
💬 maflcko commented on pull request "fuzz: Speed up PickValue in txorphan":
(https://github.com/bitcoin/bitcoin/pull/30474#discussion_r1687510289)
Thanks, done!
(https://github.com/bitcoin/bitcoin/pull/30474#discussion_r1687510289)
Thanks, done!
💬 hodlinator commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1687538297)
@paplorinc I see `hi`/`lo` is consistent with `secp256k1_uint128`, so I retract my `high` thoughts.
It strikes me that the `hi` part of your suggestion is not the one being shifted `<< 4`. Maybe it should be called `lo`?
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1687538297)
@paplorinc I see `hi`/`lo` is consistent with `secp256k1_uint128`, so I retract my `high` thoughts.
It strikes me that the `hi` part of your suggestion is not the one being shifted `<< 4`. Maybe it should be called `lo`?
💬 maflcko commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1687543213)
> it parses and makes decisions (e.g. trim, skip values after an invalid char),
No, it does not trim or skip values. Either is sets the internal state, or it does not.
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1687543213)
> it parses and makes decisions (e.g. trim, skip values after an invalid char),
No, it does not trim or skip values. Either is sets the internal state, or it does not.
💬 maflcko commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1687550108)
Thanks, fixed the comment in this test!
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1687550108)
Thanks, fixed the comment in this test!
💬 maflcko commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1687558959)
Nice, removed!
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1687558959)
Nice, removed!
💬 paplorinc commented on pull request "fuzz: Speed up PickValue in txorphan":
(https://github.com/bitcoin/bitcoin/pull/30474#issuecomment-2244466029)
utACK fa030ad6b6c124f332cea071e16c1519ae7ea423
(could you please add me as co-author, `l0rinc <pap.lorinc@gmail.com>`)?
(https://github.com/bitcoin/bitcoin/pull/30474#issuecomment-2244466029)
utACK fa030ad6b6c124f332cea071e16c1519ae7ea423
(could you please add me as co-author, `l0rinc <pap.lorinc@gmail.com>`)?
💬 maflcko commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1687574482)
> the name of the method hints at mutation, but it actually returns a copy of the view.
This isn't changed. The function signature is identical in this branch and in current master. Both take a copy of the view and return a copy of the view.
> so the copy of the parameter is implicit
No, from reading the function signature, one knows that a copy is being made when the argument is put in as a function parameter. Also, adding a `const` here does not avoid a copy or make a copy explicit in
...
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1687574482)
> the name of the method hints at mutation, but it actually returns a copy of the view.
This isn't changed. The function signature is identical in this branch and in current master. Both take a copy of the view and return a copy of the view.
> so the copy of the parameter is implicit
No, from reading the function signature, one knows that a copy is being made when the argument is put in as a function parameter. Also, adding a `const` here does not avoid a copy or make a copy explicit in
...
💬 paplorinc commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1687577476)
you're absolutely right, edited the example!
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1687577476)
you're absolutely right, edited the example!
💬 paplorinc commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1687582271)
> No, from reading the function signature, one knows that a copy is being made when the argument is put in as a function parameter
but it's an inline function, there is no function parameter.
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1687582271)
> No, from reading the function signature, one knows that a copy is being made when the argument is put in as a function parameter
but it's an inline function, there is no function parameter.
💬 vasild commented on issue "Feature Request: Broadcast Pool":
(https://github.com/bitcoin/bitcoin/issues/30471#issuecomment-2244507662)
This is related
https://github.com/bitcoin/bitcoin/pull/29415
it contains a storage of unbroadcast transactions, separate from the mempool. Transactions from that storage are periodically broadcast. A difference from this feature request is that transactions are removed from the unbroadcast storage once they are seen by the network. This can be easily changed however in a followup - for example if a local transaction is seen by the network, appears in ours and others' mempools, then keep i
...
(https://github.com/bitcoin/bitcoin/issues/30471#issuecomment-2244507662)
This is related
https://github.com/bitcoin/bitcoin/pull/29415
it contains a storage of unbroadcast transactions, separate from the mempool. Transactions from that storage are periodically broadcast. A difference from this feature request is that transactions are removed from the unbroadcast storage once they are seen by the network. This can be easily changed however in a followup - for example if a local transaction is seen by the network, appears in ours and others' mempools, then keep i
...
💬 hodlinator commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#issuecomment-2244516617)
1b7bfbab04f73ca6b9256b4e01f1eec3da5cb541 - Applied many of the suggestions from @paplorinc and @ryanofsky.
Hopefully this will achieve even more ACKs.
Added commit replacing `BOOST_CHECK()` with `BOOST_CHECK_*()` versions in **uint256_tests.cpp**.
Dropped change to `RemovePrefixView()` as @paplorinc's benchmark shows no improvement https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1686678529
Added `TxidFromString()`-test, demonstrating fix as per @ryanofsky's suggestion.
Too
...
(https://github.com/bitcoin/bitcoin/pull/30436#issuecomment-2244516617)
1b7bfbab04f73ca6b9256b4e01f1eec3da5cb541 - Applied many of the suggestions from @paplorinc and @ryanofsky.
Hopefully this will achieve even more ACKs.
Added commit replacing `BOOST_CHECK()` with `BOOST_CHECK_*()` versions in **uint256_tests.cpp**.
Dropped change to `RemovePrefixView()` as @paplorinc's benchmark shows no improvement https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1686678529
Added `TxidFromString()`-test, demonstrating fix as per @ryanofsky's suggestion.
Too
...
💬 hodlinator commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1687613008)
Dropped `RemovePrefixView()` modification.
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1687613008)
Dropped `RemovePrefixView()` modification.
💬 vasild commented on issue "Fuzzing related configuration/build options can be improved":
(https://github.com/bitcoin/bitcoin/issues/30318#issuecomment-2244529585)
Ok, that would be 1. from above https://github.com/bitcoin/bitcoin/issues/30318#issuecomment-2242965378. Just to clarify - this option would not be responsible for setting up build targets and would not add `-fsanitize=fuzzer`, right?
(https://github.com/bitcoin/bitcoin/issues/30318#issuecomment-2244529585)
Ok, that would be 1. from above https://github.com/bitcoin/bitcoin/issues/30318#issuecomment-2242965378. Just to clarify - this option would not be responsible for setting up build targets and would not add `-fsanitize=fuzzer`, right?
🤔 maflcko requested changes to a pull request: "fix: Make TxidFromString() respect string_view length"
(https://github.com/bitcoin/bitcoin/pull/30436#pullrequestreview-2193273134)
NACK. The last "refactor" push is adding new bugs or behavior changes.
Please keep "refactor" changes to the absolute minimum in real code. (Refactoring tests is fine).
(https://github.com/bitcoin/bitcoin/pull/30436#pullrequestreview-2193273134)
NACK. The last "refactor" push is adding new bugs or behavior changes.
Please keep "refactor" changes to the absolute minimum in real code. (Refactoring tests is fine).
💬 maflcko commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1687617049)
Not sure about changing code that will be obsolete in a few commits. It seems wasteful to waste review on code that will be changed or deleted down the line. It seems better to touch every line of code only once (or the least number of times possible), than to repeatedly refactor it, and then remove the "feature".
Also, this "refactor" (d38bcd39cd98dd45c8b530f415339cda2422ec9f) is introducing new bugs and behavior changes, so it seems like a regression either way.
For example, by removing
...
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1687617049)
Not sure about changing code that will be obsolete in a few commits. It seems wasteful to waste review on code that will be changed or deleted down the line. It seems better to touch every line of code only once (or the least number of times possible), than to repeatedly refactor it, and then remove the "feature".
Also, this "refactor" (d38bcd39cd98dd45c8b530f415339cda2422ec9f) is introducing new bugs and behavior changes, so it seems like a regression either way.
For example, by removing
...
💬 hodlinator commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1687632827)
`-Fragile` feels slightly off to me as well.
`-Unchecked` seems to imply that the function itself may crash.
`-Lenient` feels closer to the truth but sounds safe to use.
I recently came across `MakeRandDeterministicDANGEROUS()`, maybe something like that suffix would work?
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1687632827)
`-Fragile` feels slightly off to me as well.
`-Unchecked` seems to imply that the function itself may crash.
`-Lenient` feels closer to the truth but sounds safe to use.
I recently came across `MakeRandDeterministicDANGEROUS()`, maybe something like that suffix would work?
💬 maflcko commented on issue "Fuzzing related configuration/build options can be improved":
(https://github.com/bitcoin/bitcoin/issues/30318#issuecomment-2244565876)
> Just to clarify - this option would not be responsible for setting up build targets
I don't care, for me anything works here. Just as a reminder, if the build targets aren't set up at all, someone forgetting to specify `--target fuzz` during the build manually would run into link errors in the other targets (Because there could be conflicts between the fuzz engine's `main`, and the other targets' `main`). (I think those errors are fine, but I wanted to mention it). Also, forgetting `--targe
...
(https://github.com/bitcoin/bitcoin/issues/30318#issuecomment-2244565876)
> Just to clarify - this option would not be responsible for setting up build targets
I don't care, for me anything works here. Just as a reminder, if the build targets aren't set up at all, someone forgetting to specify `--target fuzz` during the build manually would run into link errors in the other targets (Because there could be conflicts between the fuzz engine's `main`, and the other targets' `main`). (I think those errors are fine, but I wanted to mention it). Also, forgetting `--targe
...
💬 maflcko commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1687643111)
> DANGEROUS
That may also imply the function is crashing? Maybe just `...Deprecated()`?
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1687643111)
> DANGEROUS
That may also imply the function is crashing? Maybe just `...Deprecated()`?