💬 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

🤔 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

💬 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
...

💬 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

💬 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

💬 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
...

👍 dergoegge approved a pull request: "fuzz: Speed up PickValue in txorphan"
(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.

👍 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

💬 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
...

👍 paplorinc approved a pull request: "fix: Make TxidFromString() respect string_view length"
(https://github.com/bitcoin/bitcoin/pull/30436#pullrequestreview-2193359685)
Thanks for fixing this!
ACK bfcc016825d8febf366a6b5eec5ed193c2c313bc

💬 paplorinc commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1687673170)
nit: if you edit again, consider adding @ryanofsky as co-author to the commit

💬 paplorinc commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1687693556)
nice!

💬 paplorinc commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1687739273)
+1

💬 paplorinc commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1687698438)
I wonder what this does exactly :)

💬 paplorinc commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1687749691)
+1, verified, fails as expected in previous commit
nit, could probably be simplifed to:
```suggestion
BOOST_CHECK_EQUAL(TxidFromString(std::string_view("ABCD1234", 4)),
uint256S("000000000000000000000000000000000000000000000000000000000000abcd"));
```

💬 josibake commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1687754852)
(just realised I need to update the description of this PR to be a bit more clear on the motivation)

@sipa - my main motivations for having the `KeyPair` class is to a) encapsulate the taptweak logic so that it can be used outside of signing and b) to be able to pass the `KeyPair` directly to `secp256k1_*` functions expecting a `const secp256k1_keypair *`. For the latter, I think we will need some sort of public function. Here is an example of how this used in #28201:

https://github.com/bi
...

💬 paplorinc commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1687759165)
But production code calls it indirectly, so it's not exactly deprecated, just direct access is discouraged, right?

💬 paplorinc commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1687760355)
Makes sense to do the deprecation removal in a separate PR

💬 maflcko commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1687761450)
Yes, happy to rename it to `SetHexDiscouraged`, but I don't think it matters much, as the code is deprecated to be called directly anyway.