💬 fjahr commented on pull request "test: assumeutxo: add missing tests in wallet_assumeutxo.py":
(https://github.com/bitcoin/bitcoin/pull/30455#discussion_r1679825657)
nit: should be moved up for alphabetical order

💬 sipa commented on pull request "random: add benchmarks and drop unnecessary Shuffle function":
(https://github.com/bitcoin/bitcoin/pull/30396#issuecomment-2231512385)
> I hope all code touched in this pull request is covered by fuzz tests. If not, then that should be fixed ;)

Hmm, for some reason I only considered calls to `Shuffle` / `std::shuffle` in the fuzz tests themselves. But you're right, shuffles in the code being tested would also result in fuzz inconsistency between platforms.

💬 maflcko commented on pull request "build: Introduce CMake-based buid system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1679842517)
One can see that they are indeed disabled now in https://corecheck.dev/bitcoin/bitcoin/pulls/30454 (coverage report + benchmarks).

Shouldn't autotools be nuked in this pull request, assuming that cmake and autotools are apparently incompatible, as designed now?

🤔 ryanofsky reviewed a pull request: "logging: Replace LogError and LogWarning with LogAlert"
(https://github.com/bitcoin/bitcoin/pull/30364#pullrequestreview-2180955015)
Rebased d34b529ed104a3a7e11c7d264703e61d84b1aeb3 -> 4e9ff3100ae79f3c3046a3358ff16daa7cd89627 ([`pr/alert.2`](https://github.com/ryanofsky/bitcoin/commits/pr/alert.2) -> [`pr/alert.3`](https://github.com/ryanofsky/bitcoin/commits/pr/alert.3), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/alert.2-rebase..pr/alert.3)) due to conflict with #30428

💬 ryanofsky commented on pull request "logging: Replace LogError and LogWarning with LogAlert":
(https://github.com/bitcoin/bitcoin/pull/30364#discussion_r1679841065)
re: https://github.com/bitcoin/bitcoin/pull/30364#discussion_r1675452625

> I think you can split out the 8 `/d` delete statements and three script lines for a simple last commit to just manually delete those lines

Good idea, done in latest push.

👋 maflcko's pull request is ready for review: "test: Non-Shy version sender"
(https://github.com/bitcoin/bitcoin/pull/30453)

⚠️ Ug3n3w4nd opened an issue: "Topup"
(https://github.com/bitcoin/bitcoin/issues/30460)
WXT-Wallet Adresse:
0x0495660f1a15d80b73eDc63d308Ec2f69e976C96

💬 maflcko commented on pull request "test: Non-Shy version sender":
(https://github.com/bitcoin/bitcoin/pull/30453#issuecomment-2231521003)
Disabled v2 for now, to be left as a follow-up.

✅ fanquake closed an issue: "Topup"
(https://github.com/bitcoin/bitcoin/issues/30460)

:lock: fanquake locked an issue: "Topup"
(https://github.com/bitcoin/bitcoin/issues/30460)

💬 stickies-v commented on pull request "doc: getaddressinfo[isscript] is optional":
(https://github.com/bitcoin/bitcoin/pull/30457#discussion_r1679837166)
This is a no-op, I think you meant to assert?
```suggestion
assert "isscript" not in node.getaddressinfo(BECH32_VALID_UNKNOWN_WITNESS)
```

🤔 stickies-v reviewed a pull request: "doc: getaddressinfo[isscript] is optional"
(https://github.com/bitcoin/bitcoin/pull/30457#pullrequestreview-2180948541)
code LGTM fa63ecc72045ffa771448941e1fe066f7421f640 but I think the test needs to be fixed

💬 stickies-v commented on pull request "doc: getaddressinfo[isscript] is optional":
(https://github.com/bitcoin/bitcoin/pull/30457#discussion_r1679848701)
unrelated: that is _quite_ the field description 😳 Time to start thinking about deprecating `isscript` and `iswitness`, perhaps?

💬 Sjors commented on pull request "refactor: add coinbase constraints to BlockAssembler::Options":
(https://github.com/bitcoin/bitcoin/pull/30356#discussion_r1679851292)
Currently neither of these values can be configured. With Stratum v2 I intend to add runtime checks. If those checks are wrong then I assume the fuzzer would eventually find this `Assume`?

💬 josibake commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#issuecomment-2231532258)
Updated https://github.com/bitcoin/bitcoin/commit/bdc2a656c2d2a61d226fde1d1fd4e79664106e18 -> https://github.com/bitcoin/bitcoin/commit/5e9f4f41de98c2cdcc74e31fe925b2f5cf9f6aec ([add-apply-taptweak-method-00](https://github.com/josibake/bitcoin/tree/add-apply-taptweak-method-00) -> [add-apply-taptweak-method-01](https://github.com/josibake/bitcoin/tree/add-apply-taptweak-method-01) ([compare](https://github.com/josibake/bitcoin/compare/add-apply-taptweak-method-00..josibake:add-apply-taptweak-me
...

💬 josibake commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1679854604)
Hey @paplorinc , thanks for taking a look! I agree, the first commit was quite dense. I've broken the first commit into smaller commits to aid with review and tried to include more information in the commit messages as to the motivation for the changes. Per your feedback, I've also removed the `GetKey` method as its not necessary.

💬 Sjors commented on pull request "Have createNewBlock() return a BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/30440#discussion_r1679857219)
Do you mean return `Assert(m_block_template)->block.header`?

The reason I made it a member was so that the interface would return it upon constructing `BlockTemplate`. This would then save multiple round trips over the interface later when we need a bunch of details from the header.

💬 josibake commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#issuecomment-2231544856)
cc @theuni - would appreciate your eyes on this again

💬 josibake commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1679873577)
Hey @itornaza , thanks for the review! I've incorporated your feedback and moved the comment regarding the merkle root to the `ComputeKeyPair` function and made this comment more specific to the class itself.

📝 brunoerg opened a pull request: "fuzz: add target for `CoinsResult`"
(https://github.com/bitcoin/bitcoin/pull/30461)
This PR adds fuzz coverage for `CoinsResult`.

This was addressed with another new harness proposed in https://github.com/bitcoin/bitcoin/pull/28236. However, besides the PR appearing to be abandoned (3 months since last author iteration), reviewers agreed that having a specific target for `CoinsResult` would be better.