💬 instagibbs commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1687328677)
switched to txid since that's how it's being tracked
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1687328677)
switched to txid since that's how it's being tracked
💬 instagibbs commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1687328696)
documented
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1687328696)
documented
💬 instagibbs commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1687328715)
done
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1687328715)
done
💬 davidgumberg commented on pull request "net: additional disconnect logging":
(https://github.com/bitcoin/bitcoin/pull/28521#discussion_r1687338802)
nit: Maybe this should move to the top of the function and be used in the other three disconnect logs here
```diff
if (!ShouldRunInactivityChecks(node, now)) return false;
+const std::string disconnect_msg{...};
```
(https://github.com/bitcoin/bitcoin/pull/28521#discussion_r1687338802)
nit: Maybe this should move to the top of the function and be used in the other three disconnect logs here
```diff
if (!ShouldRunInactivityChecks(node, now)) return false;
+const std::string disconnect_msg{...};
```
💬 instagibbs commented on pull request "policy: Add PayToAnchor(P2A), `OP_1 <0x4e73>` as a standard output script for spending":
(https://github.com/bitcoin/bitcoin/pull/30352#discussion_r1687367154)
ended up adding it since its required if we aren't returning the "solutions" vector
(https://github.com/bitcoin/bitcoin/pull/30352#discussion_r1687367154)
ended up adding it since its required if we aren't returning the "solutions" vector
💬 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?