💬 maflcko commented on issue "Fuzz coverage for wallet RPCs is zero":
(https://github.com/bitcoin/bitcoin/issues/30458#issuecomment-2243304292)
> Added this in #29901.
Thanks!
I'll close my issue then. Happy to review a pull request if someone creates one, but probably no need to keep this issue open in the meantime.
(https://github.com/bitcoin/bitcoin/issues/30458#issuecomment-2243304292)
> Added this in #29901.
Thanks!
I'll close my issue then. Happy to review a pull request if someone creates one, but probably no need to keep this issue open in the meantime.
💬 Sjors commented on pull request "Have createNewBlock() return a BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/30440#discussion_r1686795623)
Changed
(https://github.com/bitcoin/bitcoin/pull/30440#discussion_r1686795623)
Changed
💬 vasild commented on issue "Fuzzing related configuration/build options can be improved":
(https://github.com/bitcoin/bitcoin/issues/30318#issuecomment-2243385353)
Which are those flags?
(https://github.com/bitcoin/bitcoin/issues/30318#issuecomment-2243385353)
Which are those flags?
🚀 fanquake merged a pull request: "depends: build zeromq with CMake"
(https://github.com/bitcoin/bitcoin/pull/29723)
(https://github.com/bitcoin/bitcoin/pull/29723)
💬 sipa commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1686762705)
In commit "tests: add key tweak smoke test"
The `merkle_root.IsNull()` condition will never hold here. Perhaps it makes sense to run this test in a loop, and only set `merkle_root` to `InsecureRand256()` in some of the iterations?
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1686762705)
In commit "tests: add key tweak smoke test"
The `merkle_root.IsNull()` condition will never hold here. Perhaps it makes sense to run this test in a loop, and only set `merkle_root` to `InsecureRand256()` in some of the iterations?
💬 sipa commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1686843355)
If the only purpose of this function is constructing a `const secp256k1_keypair*`, I think it would be better to make it private (it seems only accessed inside `KeyPair` anyway?), and give it another name (`data()` is used in STL containers to give raw access to the contents, which seems to be explicitly what we don't want here).
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1686843355)
If the only purpose of this function is constructing a `const secp256k1_keypair*`, I think it would be better to make it private (it seems only accessed inside `KeyPair` anyway?), and give it another name (`data()` is used in STL containers to give raw access to the contents, which seems to be explicitly what we don't want here).
🚀 fanquake merged a pull request: "lint: Add missing docker.io prefix to ci/lint_imagefile"
(https://github.com/bitcoin/bitcoin/pull/30501)
(https://github.com/bitcoin/bitcoin/pull/30501)
🚀 fanquake merged a pull request: "Fix lint-spelling warnings"
(https://github.com/bitcoin/bitcoin/pull/30500)
(https://github.com/bitcoin/bitcoin/pull/30500)
👍 fanquake approved a pull request: "depends: Fix CMake-generated `libevent*.pc` files"
(https://github.com/bitcoin/bitcoin/pull/30488#pullrequestreview-2192092719)
ACK 8c935e625ea75d180144f0526d6a0d5fd58c1f29
(https://github.com/bitcoin/bitcoin/pull/30488#pullrequestreview-2192092719)
ACK 8c935e625ea75d180144f0526d6a0d5fd58c1f29
🚀 fanquake merged a pull request: "depends: Fix CMake-generated `libevent*.pc` files"
(https://github.com/bitcoin/bitcoin/pull/30488)
(https://github.com/bitcoin/bitcoin/pull/30488)
👍 dergoegge approved a pull request: "fuzz: reduce keypool size in `scriptpubkeyman` target"
(https://github.com/bitcoin/bitcoin/pull/30494#pullrequestreview-2192109414)
utACK dcb4ec944984961e3b40452425a219e15e6ab508
(https://github.com/bitcoin/bitcoin/pull/30494#pullrequestreview-2192109414)
utACK dcb4ec944984961e3b40452425a219e15e6ab508
✅ fanquake closed an issue: "scriptpubkeyman fuzz target TopUp is slow"
(https://github.com/bitcoin/bitcoin/issues/30476)
(https://github.com/bitcoin/bitcoin/issues/30476)
🚀 fanquake merged a pull request: "fuzz: reduce keypool size in `scriptpubkeyman` target"
(https://github.com/bitcoin/bitcoin/pull/30494)
(https://github.com/bitcoin/bitcoin/pull/30494)
💬 theStack commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1686876369)
> Spent some time thinking about this and realised the main issue here is we start with a valid secret key, create a keypair (which generates the public key in the process), and then extract the public key from the keypair and serialise it. Instead, we can just get the public key directly from the secret key via `CKey::GetPubKey`. This makes the original code more clear, eliminates the unnecessary if branches and makes the diff _much_ simpler.
>
Not sure if we care that much about signing s
...
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1686876369)
> Spent some time thinking about this and realised the main issue here is we start with a valid secret key, create a keypair (which generates the public key in the process), and then extract the public key from the keypair and serialise it. Instead, we can just get the public key directly from the secret key via `CKey::GetPubKey`. This makes the original code more clear, eliminates the unnecessary if branches and makes the diff _much_ simpler.
>
Not sure if we care that much about signing s
...
💬 1440000bytes commented on pull request "policy: enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/30493#issuecomment-2243571732)
Thanks @reardencode for the review and ACK as its obvious full-rbf should be default. There is some politics involved in it which I wanted to highlight:
---

I don't know the reason for deleting this comments and others who reacted with emojis, however it will be better for bitcoin if we can get some ACKs to merge it. I did not mention the vulnerability in rationale because other things are sufficie
...
(https://github.com/bitcoin/bitcoin/pull/30493#issuecomment-2243571732)
Thanks @reardencode for the review and ACK as its obvious full-rbf should be default. There is some politics involved in it which I wanted to highlight:
---

I don't know the reason for deleting this comments and others who reacted with emojis, however it will be better for bitcoin if we can get some ACKs to merge it. I did not mention the vulnerability in rationale because other things are sufficie
...
👍 itornaza approved a pull request: "crypto, refactor: add new KeyPair class"
(https://github.com/bitcoin/bitcoin/pull/30051#pullrequestreview-2192299471)
trACK 9afa2c9e50370b2918377f3f3eac0950a4296ec0
Reviewed the changes in the `key_tests.cpp` since my last review on this PR. Again, all unit and functional including the extended tests pass. Just dropped a name candidate if you consider renaming `KeyPair::data()` and run out of options.
(https://github.com/bitcoin/bitcoin/pull/30051#pullrequestreview-2192299471)
trACK 9afa2c9e50370b2918377f3f3eac0950a4296ec0
Reviewed the changes in the `key_tests.cpp` since my last review on this PR. Again, all unit and functional including the extended tests pass. Just dropped a name candidate if you consider renaming `KeyPair::data()` and run out of options.
💬 itornaza commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1686990120)
If there is not a better alternative, I would suggest `secp256k1_keypair()` as a candidate for renaming this `data()` member function to further signify its use through the cast and be called like so:
`reinterpret_cast<const secp256k1_keypair*>(keypair.secp256k1_keypair())`
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1686990120)
If there is not a better alternative, I would suggest `secp256k1_keypair()` as a candidate for renaming this `data()` member function to further signify its use through the cast and be called like so:
`reinterpret_cast<const secp256k1_keypair*>(keypair.secp256k1_keypair())`
💬 murchandamus commented on pull request "policy: enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/30493#issuecomment-2243587592)
Concept ACK
Given the majority of the hashrate using `mempoolfullrbf`, Bitcoin Core should adapt its default behavior to match its expectations to the blocks being published.
(https://github.com/bitcoin/bitcoin/pull/30493#issuecomment-2243587592)
Concept ACK
Given the majority of the hashrate using `mempoolfullrbf`, Bitcoin Core should adapt its default behavior to match its expectations to the blocks being published.
💬 murchandamus commented on pull request "policy: enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/30493#discussion_r1686996487)
This file is named incorrectly.
(https://github.com/bitcoin/bitcoin/pull/30493#discussion_r1686996487)
This file is named incorrectly.
👋 naiyoma's pull request is ready for review: "Test/rpc whitelistdefault test"
(https://github.com/bitcoin/bitcoin/pull/29858)
(https://github.com/bitcoin/bitcoin/pull/29858)