Bitcoin Core Github
43 subscribers
123K links
Download Telegram
💬 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.
🚀 fanquake merged a pull request: "fuzz: Speed up PickValue in txorphan"
(https://github.com/bitcoin/bitcoin/pull/30474)
💬 paplorinc commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1687771391)
will leave it up to you, but if I see a `Set.*` I assume it's a property setter (which has an equivalent Getter), not that something is calculated and internal state (maybe) modified accordingly.
💬 paplorinc commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1687765720)
I'm fine with that if you are
💬 paplorinc commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1687764790)
you may want to revert it in the commit that introduced it instead
💬 maflcko commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1687776017)
> will leave it up to you, but if I see a `Set.*` I assume it's a property setter (which has an equivalent Getter)

Yes, there is a corresponding `GetHex`.
💬 maflcko commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1687777514)
> you may want to revert it in the commit that introduced it instead

It is a scripted-diff, so it is correct and self-consistent, no?
💬 paplorinc commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1687781534)
ah, got it, sure!
maflcko closed an issue: "descriptor: Tapscript-specific Miniscript key serialization / parsing leads to fuzz timeouts"
(https://github.com/bitcoin/bitcoin/issues/30168)
💬 maflcko commented on issue "descriptor: Tapscript-specific Miniscript key serialization / parsing leads to fuzz timeouts":
(https://github.com/bitcoin/bitcoin/issues/30168#issuecomment-2244797028)
This was fixed in commit 262260ce1e919613ba60194a5861b0b0a51dfe01
💬 paplorinc commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1687798251)
You mean it's a "virtual property" - I can work with that, thanks, resolve it please.
(though it's weird that the getter seems to reverse the hex back to big-endian, weird, but I guess that's for another PR)
💬 hodlinator commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1687805203)
Indeed.. saw it too. Seems like temporary code that somehow made it into the test. Guess I could remove if I retouch.
⚠️ maflcko opened an issue: "fuzz: crypter timeout"
(https://github.com/bitcoin/bitcoin/issues/30503)
Found by oss-fuzz

[clusterfuzz-testcase-crypter-4690510954954752.bin.not.txt](https://github.com/user-attachments/files/16347247/clusterfuzz-testcase-crypter-4690510954954752.bin.not.txt)


```
FUZZ=crypter perf record -g --call-graph dwarf ./src/test/fuzz/fuzz ./clusterfuzz-testcase-crypter-4690510954954752
... half a minute ...
hotspot ./perf.data
```


![Screenshot from 2024-07-23 12-16-34](https://github.com/user-attachments/assets/f37f17e9-2b38-475a-8f59-caedf29c30c8)
💬 maflcko commented on issue "fuzz: crypter timeout":
(https://github.com/bitcoin/bitcoin/issues/30503#issuecomment-2244828423)
My recommendation would be to limit the number of iterations to something reasonable, but still enough to cover all cases.
💬 theStack commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1687820299)
Fwiw, we have a signing benchmark for taproot key-path spends, haven't verified yet if it covers the discussed code path though: https://github.com/bitcoin/bitcoin/blob/910d38b22f575cba9a3325de3f4c5ac667d4a487/src/bench/sign_transaction.cpp#L67

> In this case, I do think improving the clarity of the code and removing extra steps at the cost of performance seems worth it? I can also write a benchmark to quantify if performance here is indeed a concern.

No hard feelings on that topic from my
...
📝 vasild opened a pull request: "doc: use proper doxygen formatting for CTxMemPool::cs"
(https://github.com/bitcoin/bitcoin/pull/30504)
Having `@par title` followed by an empty line renders improperly in Doxygen - it results in a paragraph with a title but without a body.

https://www.doxygen.nl/manual/commands.html#cmdpar

This also results in a compiler warning (or error) with Clang 19:

```
./txmempool.h:368:34: error: empty paragraph passed to '@par' command [-Werror,-Wdocumentation]
368 | * @par Consistency guarantees
| ~~~~~~~~~~~~~~~~~~~~~~~~~~^
1 error generated.
```

<!--
*** Please rem
...
💬 fanquake commented on pull request "depends: build expat with CMake":
(https://github.com/bitcoin/bitcoin/pull/29878#issuecomment-2244849619)
Guix Build (aarch64):
```bash
4ed36603100265640e4e9e73fa8f04c2b10a581c0a68f37cb636f1cbff71469c guix-build-cf46c6b8f2ff/output/aarch64-linux-gnu/SHA256SUMS.part
70b0f40dec0646cc7378c17bb569c8aa292ceba93e9b7233e7a0067fde04a24c guix-build-cf46c6b8f2ff/output/aarch64-linux-gnu/bitcoin-cf46c6b8f2ff-aarch64-linux-gnu-debug.tar.gz
e778fdbf6dcf74a8cfeac8d3b353f877809f96a6f4759e497d939b001874a357 guix-build-cf46c6b8f2ff/output/aarch64-linux-gnu/bitcoin-cf46c6b8f2ff-aarch64-linux-gnu.tar.gz
c99a80
...
⚠️ maflcko opened an issue: "fuzz: crypto_fschacha20poly1305 timeout"
(https://github.com/bitcoin/bitcoin/issues/30505)
Found by oss-fuzz

[clusterfuzz-testcase-crypto_fschacha20poly1305-6583678709334016.txt](https://github.com/user-attachments/files/16347368/clusterfuzz-testcase-crypto_fschacha20poly1305-6583678709334016.txt)


```
FUZZ=crypto_fschacha20poly1305 perf record -g --call-graph dwarf ./src/test/fuzz/fuzz ./clusterfuzz-testcase-crypto_fschacha20poly1305-6583678709334016
...
hotspot ./perf.data
```

![Screenshot from 2024-07-23 12-34-02](https://github.com/user-attachments/assets/3111eab4-
...