⚠️ 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.

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

No I think the diff above should work. There should be no need for calling GetBlockHeader() at all, much less caching the result of the call, because CBlock inherits from CBlockHeader.

I'm not suggesting dropping the getBlockHeader interface method, because that method is useful for avoiding round trips like you suggest.

💬 Sjors commented on pull request "Silent payment index (for light wallets and consistency check)":
(https://github.com/bitcoin/bitcoin/pull/28241#issuecomment-2231592557)
Matches!

💬 Sjors commented on pull request "Have createNewBlock() return a BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/30440#discussion_r1679915895)
> because CBlock inherits from CBlockHeader

Ah ok

> that method is useful for avoiding round trips

The choice is between having the caller be careful to avoid round trips, vs not needing round trips at all because we already got the header at construction time.

That's likely a tiny difference, so whatever is better code is probably best.

📝 theuni opened a pull request: "utils: replace boost::date_time usage with c++20 std::chrono"
(https://github.com/bitcoin/bitcoin/pull/30462)
This would be a very straightforward and uncontroversial change if not for the sad state of std lib implementations which implement this functionality.

As of now, `std::chrono::parse` and friends (c++20 additions) are only available as of gcc 14 and (unreleased) clang 19.

Formatting in chrono is quite useful, so I don't think it makes sense to wait years until we require those compilers.

Instead of waiting around, this PR takes the sledgehammer approach of adding a fully-conformant impl
...

💬 theuni commented on pull request "utils: replace boost::date_time usage with c++20 std::chrono":
(https://github.com/bitcoin/bitcoin/pull/30462#issuecomment-2231641407)
This was noticed while working on #30434, which requires us to install `boost::date_time`. Though vendoring `date.h` here may seem heavy, keep in mind that the status quo is a forward-incompatible mess of boost headers :)

💬 hebasto commented on pull request "utils: replace boost::date_time usage with c++20 std::chrono":
(https://github.com/bitcoin/bitcoin/pull/30462#issuecomment-2231641415)
> This allows us to get rid of our last use of `boost::date_time`

Concept ACK on that.