💬 hodlinator commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1687648309)
> 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".
The function is not being removed as of #30482.
> For example, by removing the `std::fill(m_data.begin(), m_data.end(), 0);`, two `SetHex` calls now may result in a different v
...
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1687648309)
> 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".
The function is not being removed as of #30482.
> For example, by removing the `std::fill(m_data.begin(), m_data.end(), 0);`, two `SetHex` calls now may result in a different v
...
💬 maflcko commented on issue "Feature Request: Broadcast Pool":
(https://github.com/bitcoin/bitcoin/issues/30471#issuecomment-2244580379)
> The broadcast pool _only_ contains transactions that Alice has broadcast either from her Core wallet or via the `sendrawtransaction` RPC.
I see. I assumed that Mallory and Alice could have wallet on the same node (multiwallet mode), or that they shared the `sendrawtransaction` RPC on the same node. Not sure if this assumption is accurate, but it seemed plausible that some nodes expose their `sendrawtransaction` to third parties.
(https://github.com/bitcoin/bitcoin/issues/30471#issuecomment-2244580379)
> The broadcast pool _only_ contains transactions that Alice has broadcast either from her Core wallet or via the `sendrawtransaction` RPC.
I see. I assumed that Mallory and Alice could have wallet on the same node (multiwallet mode), or that they shared the `sendrawtransaction` RPC on the same node. Not sure if this assumption is accurate, but it seemed plausible that some nodes expose their `sendrawtransaction` to third parties.
💬 maflcko commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1687659582)
I see, the code may be fine then, but I still find it odd to change a line of code repeatedly over time when it can be changed once.
> The function is not being removed as of #30482.
Correct, but parsing less (or more) digits that the data structure can hold does not make sense, so seems best to remove before refactoring this "feature". If there is a use-case, it would be good to mention it, so that it is clear that the refactor is the last time this function needs to be touched.
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1687659582)
I see, the code may be fine then, but I still find it odd to change a line of code repeatedly over time when it can be changed once.
> The function is not being removed as of #30482.
Correct, but parsing less (or more) digits that the data structure can hold does not make sense, so seems best to remove before refactoring this "feature". If there is a use-case, it would be good to mention it, so that it is clear that the refactor is the last time this function needs to be touched.
💬 maflcko commented on pull request "fuzz: Speed up PickValue in txorphan":
(https://github.com/bitcoin/bitcoin/pull/30474#issuecomment-2244600650)
> (could you please add me as co-author, `l0rinc <pap.lorinc@gmail.com>`?)
Sure, done
(https://github.com/bitcoin/bitcoin/pull/30474#issuecomment-2244600650)
> (could you please add me as co-author, `l0rinc <pap.lorinc@gmail.com>`?)
Sure, done
💬 hodlinator commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1687667202)
It is dangerous because input isn't length-checked, meaning extra hex chars may be ignored, and an accidentally short copy-paste of hex chars will be silently padded with zeroes.
> Maybe just ...Deprecated()?
As in https://github.com/bitcoin/bitcoin/pull/30482?notification_referrer_id=NT_kwDOCkdNarUxMTU3NjYyODE4MzoxNzI0NDUwMzQ#discussion_r1685726873, I wonder what the policy around deprecation is. Are there prior examples of deprecation on a non-RPC level you can point to?
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1687667202)
It is dangerous because input isn't length-checked, meaning extra hex chars may be ignored, and an accidentally short copy-paste of hex chars will be silently padded with zeroes.
> Maybe just ...Deprecated()?
As in https://github.com/bitcoin/bitcoin/pull/30482?notification_referrer_id=NT_kwDOCkdNarUxMTU3NjYyODE4MzoxNzI0NDUwMzQ#discussion_r1685726873, I wonder what the policy around deprecation is. Are there prior examples of deprecation on a non-RPC level you can point to?
👍 TheCharlatan approved a pull request: "log: Remove NOLINT(bitcoin-unterminated-logprintf)"
(https://github.com/bitcoin/bitcoin/pull/30485#pullrequestreview-2193365252)
ACK fa18fc705084f3620be566d8c6639b29117ccf68
(https://github.com/bitcoin/bitcoin/pull/30485#pullrequestreview-2193365252)
ACK fa18fc705084f3620be566d8c6639b29117ccf68
💬 maflcko commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1687679779)
Only `src/util/time.h`, but those are just type-related; And `src/logging.h`, but those are just aliases; So I don't think they apply here.
Happy to rename it to `SetHexDeprecated`, but I don't think it matters much, as the code is deprecated anyway.
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1687679779)
Only `src/util/time.h`, but those are just type-related; And `src/logging.h`, but those are just aliases; So I don't think they apply here.
Happy to rename it to `SetHexDeprecated`, but I don't think it matters much, as the code is deprecated anyway.
💬 fanquake commented on pull request "depends: build expat with CMake":
(https://github.com/bitcoin/bitcoin/pull/29878#issuecomment-2244662144)
> should we mirror this behaviour?
Which behaviour? If this is missing from one build system, that could be a bug that should be reported upstream. Given that these are stubs compiled just so Qt can compile, I don't think it matters too much.
> Should we bump the CMake minimum required version
Sure. Added a patch.
(https://github.com/bitcoin/bitcoin/pull/29878#issuecomment-2244662144)
> should we mirror this behaviour?
Which behaviour? If this is missing from one build system, that could be a bug that should be reported upstream. Given that these are stubs compiled just so Qt can compile, I don't think it matters too much.
> Should we bump the CMake minimum required version
Sure. Added a patch.
💬 craigraw commented on issue "Feature Request: Broadcast Pool":
(https://github.com/bitcoin/bitcoin/issues/30471#issuecomment-2244671905)
@vaslid This sounds excellent. From your comment it seems like https://github.com/bitcoin/bitcoin/pull/29415 is a great basis to implement this. I will look into that work more closely, as it's very useful in it's own right. Currently, if Tor is configured, Sparrow broadcasts over Tor via a random external service such as mempool.space for greater privacy. As I understand it, this would no longer be necessary with `-privatebroadcast=1`.
> It is a separate task, which I am planning after https
...
(https://github.com/bitcoin/bitcoin/issues/30471#issuecomment-2244671905)
@vaslid This sounds excellent. From your comment it seems like https://github.com/bitcoin/bitcoin/pull/29415 is a great basis to implement this. I will look into that work more closely, as it's very useful in it's own right. Currently, if Tor is configured, Sparrow broadcasts over Tor via a random external service such as mempool.space for greater privacy. As I understand it, this would no longer be necessary with `-privatebroadcast=1`.
> It is a separate task, which I am planning after https
...
👍 maflcko approved a pull request: "test, assumeutxo: Remove resolved todo comments and add new test"
(https://github.com/bitcoin/bitcoin/pull/30403#pullrequestreview-2193413701)
ACK d63ef738001fb69ce04134cc8645dcd1e1cbccd1
left a nit (possibly a new test idea, but this can be done later).
(https://github.com/bitcoin/bitcoin/pull/30403#pullrequestreview-2193413701)
ACK d63ef738001fb69ce04134cc8645dcd1e1cbccd1
left a nit (possibly a new test idea, but this can be done later).
💬 maflcko commented on pull request "test, assumeutxo: Remove resolved todo comments and add new test":
(https://github.com/bitcoin/bitcoin/pull/30403#discussion_r1687708137)
nit: I think that test in https://github.com/bitcoin/bitcoin/pull/30267 only checks for a known-invalid header, not for a valid header of a block that later "turns out to be invalid". Also, a test could be added where a block later turns out to have less work, but this is discussed in https://github.com/bitcoin/bitcoin/issues/30288
(https://github.com/bitcoin/bitcoin/pull/30403#discussion_r1687708137)
nit: I think that test in https://github.com/bitcoin/bitcoin/pull/30267 only checks for a known-invalid header, not for a valid header of a block that later "turns out to be invalid". Also, a test could be added where a block later turns out to have less work, but this is discussed in https://github.com/bitcoin/bitcoin/issues/30288
🤔 glozow reviewed a pull request: "fuzz: Speed up PickValue in txorphan"
(https://github.com/bitcoin/bitcoin/pull/30474#pullrequestreview-2193432107)
ACK fa33a63bd9458f3487a0592983c1363cd30a3c74, thanks for taking the suggestion
(https://github.com/bitcoin/bitcoin/pull/30474#pullrequestreview-2193432107)
ACK fa33a63bd9458f3487a0592983c1363cd30a3c74, thanks for taking the suggestion
💬 alfonsoromanz commented on pull request "test: assumeutxo: add missing tests in wallet_assumeutxo.py":
(https://github.com/bitcoin/bitcoin/pull/30455#discussion_r1687725095)
Thanks for the feedback. I added the `fastprune` option to my local copy and I'm working to understand how pruning operates.
When I tried loading the wallet during background synchronization ([here](https://github.com/bitcoin/bitcoin/pull/30455/files#diff-f763a7cf53e7a5183f66f2634baef84245919265a09d82b03dc435adea8b8faeR149)), I noticed that there wasn't a pruneheight set yet. So, I did a manual prune using `n2.pruneblockchain(START_HEIGHT)`. After pruning, I saw a pruneheight of 300, and the
...
(https://github.com/bitcoin/bitcoin/pull/30455#discussion_r1687725095)
Thanks for the feedback. I added the `fastprune` option to my local copy and I'm working to understand how pruning operates.
When I tried loading the wallet during background synchronization ([here](https://github.com/bitcoin/bitcoin/pull/30455/files#diff-f763a7cf53e7a5183f66f2634baef84245919265a09d82b03dc435adea8b8faeR149)), I noticed that there wasn't a pruneheight set yet. So, I did a manual prune using `n2.pruneblockchain(START_HEIGHT)`. After pruning, I saw a pruneheight of 300, and the
...
💬 alfonsoromanz commented on pull request "test: assumeutxo: add missing tests in wallet_assumeutxo.py":
(https://github.com/bitcoin/bitcoin/pull/30455#discussion_r1687725271)
Thanks. Will fix it in my next push
(https://github.com/bitcoin/bitcoin/pull/30455#discussion_r1687725271)
Thanks. Will fix it in my next push
💬 alfonsoromanz commented on pull request "test: assumeutxo: add missing tests in wallet_assumeutxo.py":
(https://github.com/bitcoin/bitcoin/pull/30455#discussion_r1687727279)
Thanks. I will address this in my next push along with the other feedback from fjahr
(https://github.com/bitcoin/bitcoin/pull/30455#discussion_r1687727279)
Thanks. I will address this in my next push along with the other feedback from fjahr
💬 josibake commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1687728635)
Yep, this will be slower (in cases where we don't have a merkle root and instead use the public key per BIP341's recommendation). I looked to see if we had a benchmark for signing to see if I could quantify this, but didn't see one. Was thinking about writing one but then talked myself out of it because I don't think performance is as much a concern in signing (as opposed to verification, where it absolutely is a concern).
In this case, I do think improving the clarity of the code and removin
...
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1687728635)
Yep, this will be slower (in cases where we don't have a merkle root and instead use the public key per BIP341's recommendation). I looked to see if we had a benchmark for signing to see if I could quantify this, but didn't see one. Was thinking about writing one but then talked myself out of it because I don't think performance is as much a concern in signing (as opposed to verification, where it absolutely is a concern).
In this case, I do think improving the clarity of the code and removin
...
👍 dergoegge approved a pull request: "fuzz: Speed up PickValue in txorphan"
(https://github.com/bitcoin/bitcoin/pull/30474#pullrequestreview-2193455178)
utACK fa33a63bd9458f3487a0592983c1363cd30a3c74
(https://github.com/bitcoin/bitcoin/pull/30474#pullrequestreview-2193455178)
utACK fa33a63bd9458f3487a0592983c1363cd30a3c74
💬 fanquake commented on pull request "guix: bump time-machine to 1aa8dfaeec3c6e4e587aadf7440246f7c5c04b9f":
(https://github.com/bitcoin/bitcoin/pull/30452#issuecomment-2244715566)
This is the problematic commit: https://github.com/mingw-w64/mingw-w64/commit/9a80e57890f887afd60e0b019d1c6698351fd470. Which, by no-longer using it's internal definitions, causes GCC to reach for intrinsics , which left a (different per arch) path to the `/gnu/store/` in the debug info. This pointed out a different issue, which is that the path was to GCC 11 libs, rather than 12. I'll open a new PR to fix winpthreads being compiled with 11 rather than 12, and then come back here.
(https://github.com/bitcoin/bitcoin/pull/30452#issuecomment-2244715566)
This is the problematic commit: https://github.com/mingw-w64/mingw-w64/commit/9a80e57890f887afd60e0b019d1c6698351fd470. Which, by no-longer using it's internal definitions, causes GCC to reach for intrinsics , which left a (different per arch) path to the `/gnu/store/` in the debug info. This pointed out a different issue, which is that the path was to GCC 11 libs, rather than 12. I'll open a new PR to fix winpthreads being compiled with 11 rather than 12, and then come back here.
👍 alfonsoromanz approved a pull request: "test, assumeutxo: Remove resolved todo comments and add new test"
(https://github.com/bitcoin/bitcoin/pull/30403#pullrequestreview-2193474993)
Re ACK d63ef738001fb69ce04134cc8645dcd1e1cbccd1
(https://github.com/bitcoin/bitcoin/pull/30403#pullrequestreview-2193474993)
Re ACK d63ef738001fb69ce04134cc8645dcd1e1cbccd1
💬 paplorinc commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1687692528)
nit (is a bit more verbose, but gives way better error during failure):
```patch
diff --git a/src/test/uint256_tests.cpp b/src/test/uint256_tests.cpp
--- a/src/test/uint256_tests.cpp (revision bfcc016825d8febf366a6b5eec5ed193c2c313bc)
+++ b/src/test/uint256_tests.cpp (date 1721726677844)
@@ -169,11 +169,11 @@
TmpL.SetHex(ZeroL.ToString()); BOOST_CHECK_EQUAL(TmpL, uint256());
TmpL.SetHex(R1L.ToString());
- BOOST_CHECK_EQUAL(memcmp(R1L.begin(), R1Array, 32), 0);
- BOOST
...
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1687692528)
nit (is a bit more verbose, but gives way better error during failure):
```patch
diff --git a/src/test/uint256_tests.cpp b/src/test/uint256_tests.cpp
--- a/src/test/uint256_tests.cpp (revision bfcc016825d8febf366a6b5eec5ed193c2c313bc)
+++ b/src/test/uint256_tests.cpp (date 1721726677844)
@@ -169,11 +169,11 @@
TmpL.SetHex(ZeroL.ToString()); BOOST_CHECK_EQUAL(TmpL, uint256());
TmpL.SetHex(R1L.ToString());
- BOOST_CHECK_EQUAL(memcmp(R1L.begin(), R1Array, 32), 0);
- BOOST
...