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

💬 josibake commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1687763266)
This is how @paplorinc originally proposed the test: https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685696117

I felt like computing multiple keys in a loop was overkill and removed the merkle root switching since that's not really what's being tested here. The goal here is to sanity check that we get the same public key bytes from GetPubKey() as we would get directly accessing the `secp256k1_keypair` using the `_xonly_pub` and `_serialize`.

So perhaps better to just remove the
...

💬 maflcko commented on pull request "guix: use GCC 13 to builds releases":
(https://github.com/bitcoin/bitcoin/pull/29881#discussion_r1687766168)
I ran the task 10 times, and it failed consistently, so I don't think it is the same bug:

https://cirrus-ci.com/task/4925521652350976

📝 vasild opened a pull request: "doc: add release notes for #22729"
(https://github.com/bitcoin/bitcoin/pull/30502)
Add release notes for #22729.