💬 Sjors commented on pull request "Stratum v2 Noise Protocol":
(https://github.com/bitcoin/bitcoin/pull/29346#discussion_r1673779622)
Done

💬 Sjors commented on pull request "Stratum v2 Noise Protocol":
(https://github.com/bitcoin/bitcoin/pull/29346#issuecomment-2222579946)
Rebased after silent merge conflict with #29625 which moved `xoroshiro128plusplus.h`. Addressed comments by @itornaza.

💬 fjahr commented on pull request "Stratum v2 Template Provider (take 3)":
(https://github.com/bitcoin/bitcoin/pull/29432#issuecomment-2222600328)
I am just catching up on this thread so I haven't really made up my mind on the conceptual question overall. But if we could wave our magic wand to have multiprocess plus sv2 based on that that sounds like a great achievement for the project overall. Adding multiprocess support would take its time though, these would then be two large projects that have been around for a long time but struggled to get enough review power. If sv2 could help move multiprocess forward that would be great but given
...

📝 glozow opened a pull request: "remove truc_policy from libbitcoin_common_a_SOURCES"
(https://github.com/bitcoin/bitcoin/pull/30427)
Hebasto pointed out that it doesn't need to be there since it's in `libbitcoin_node_a_SOURCES`

💬 josibake commented on pull request "Stratum v2 Template Provider (take 3)":
(https://github.com/bitcoin/bitcoin/pull/29432#issuecomment-2222638193)
It seems to me two topics are being debated here, one about software design and the other about how we get users to run software.

## Technical design

I agree with @dergoegge that there have been no _technical_ objections to a multi-process approach, i.e., a `stratumv2` sidecar talking to `bitcoind` via some sort of communication protocol. Instead, I find the arguments for a multi-process architecture to be compelling and the correct _technical_ design. Specifically, exposing a somewhat gen
...

💬 maflcko commented on pull request "remove truc_policy from libbitcoin_common_a_SOURCES":
(https://github.com/bitcoin/bitcoin/pull/30427#issuecomment-2222672689)
In theory `libbitcoin_wallet` may want to use it at some point? Though, I guess it can be moved back then, if needed.

💬 fjahr commented on pull request "Stratum v2 Template Provider (take 3)":
(https://github.com/bitcoin/bitcoin/pull/29432#issuecomment-2222675998)
> It seems to me two topics are being debated here, one about software design and the other about how we get users to run software

I think this is mostly about engineering resources in Bitcoin Core

💬 hebasto commented on pull request "remove truc_policy from libbitcoin_common_a_SOURCES":
(https://github.com/bitcoin/bitcoin/pull/30427#issuecomment-2222679596)
> In theory `libbitcoin_wallet` may want to use it at some point? Though, I guess it can be moved back then, if needed.

If/when it happens, the `policy/truc_policy.cpp` source file can be moved to the `libbitcoin_common.a` static library sources.

👍 hebasto approved a pull request: "remove truc_policy from libbitcoin_common_a_SOURCES"
(https://github.com/bitcoin/bitcoin/pull/30427#pullrequestreview-2171630025)
ACK e8c3b7172c33929e4e5bf6059da2d25a4ea8779c, this change follows the design [docs](https://github.com/bitcoin/bitcoin/blob/master/doc/design/libraries.md).

👍 stickies-v approved a pull request: "refactor: modernize-use-equals-default"
(https://github.com/bitcoin/bitcoin/pull/30406#pullrequestreview-2171633506)
ACK 3333bae9b2a6c1ee2314d33361c93944c12001f9

My understanding is that prior to C++20, aggregates could not have user-provided constructors and destructors, but could have user-declared ones. Since C++20, the requirements to be an aggregate have been tightened to not allow user-declared ctors and dtors either. As such, this proposed change does not affect initialization of the affected structs and classes.

I would think the benefits are mild, but I don't see any real downsides either - easy
...

💬 maflcko commented on pull request "refactor: modernize-use-equals-default":
(https://github.com/bitcoin/bitcoin/pull/30406#issuecomment-2222716625)
> I would think the benefits are mild, but I don't see any real downsides either - easy to review, and minimal (easy-to-resolve) conflicts. Added friction for developers seems minimal too, and issues are trivially resolved after CI tidy failure.

Agree that the performance benefits are mild at best, if any at all. At this point it is mostly for consistency, because most of the codebase already uses `=default`. Also, it makes meta-programming easier, as well as writing new tidy checks, knowing
...

💬 maflcko commented on pull request "rpc, rest: Improve getblock error when only header is available":
(https://github.com/bitcoin/bitcoin/pull/30410#issuecomment-2222779382)
> Ok, I'll give it a try.

Ok, I am done, but I think it is easier to review this pull request.




> > However, you forgot GetBlockChecked (and all the other functions that can fail in the same way)?
>
> Updated to change all functions I could find. I was only looking at `getblock` before, not at other RPCs.

Still missing:

* `blockToJSON`
* `getrawtransaction`
* `gettxoutproof`

I think you can just expose the helper functions and use them everywhere?

💬 Sjors commented on pull request "Stratum v2 Template Provider (take 3)":
(https://github.com/bitcoin/bitcoin/pull/29432#issuecomment-2222789717)
Rebased after #29274 was merged. Now using the new `waitTipChanged()` interface method from #30409: faf0c8ec1701d0b497e962033477d2aed8c556c3 -> c3a0a51aece3f9174be80eea81795140bb2415b3 is a nice simplification in `ThreadSv2Handler()`.

💬 Sjors commented on pull request "Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange and drop CRPCSignals":
(https://github.com/bitcoin/bitcoin/pull/30409#discussion_r1673937990)
I changed this in the last push, previously it would always wait for one second each "round" even if the deadline was shorter.

💬 fanquake commented on pull request "contrib: simplify `test-security-check`":
(https://github.com/bitcoin/bitcoin/pull/30423#issuecomment-2222844502)
> So what kind of "isolation" do you mean?

Testing one thing per test.

👍 ismaelsadeeq approved a pull request: "test: fix inconsistency in fundrawtransaction weight limits test"
(https://github.com/bitcoin/bitcoin/pull/30353#pullrequestreview-2171797056)
Code review and Tested ACK 00b8e26bd6a47a8207c7571ca7ef9643f0ee2668

I was able to recreate the failure with the diff in the OP. I also decoded the transaction and confirmed that it's selecting the change output of the parent transaction with a value of 2.79949465 BTC. That's why the test was not failing.

The commit 00b8e26bd6a47a8207c7571ca7ef9643f0ee2668 fixes this issue.

> This is not a problem for `fundrawtransaction()`

Yes, it is not. The call will succeed, but when attempting to
...

💬 glozow commented on pull request "locks: introduce mutex for tx download, flush rejection filters on UpdatedBlockTip":
(https://github.com/bitcoin/bitcoin/pull/30111#discussion_r1673957302)
That seems incorrect. `ActivateBestchain` exits before `ActiveTipChange` if it realizes there is nothing to do, e.g. if `invalidateblock` only causes the tip to change from block n to block n-1.

https://github.com/bitcoin/bitcoin/blob/9b480f7a25a737c9c4ebc33401e94d66c2da9ec3/src/validation.cpp#L3505-L3508

🤔 ismaelsadeeq reviewed a pull request: "remove truc_policy from libbitcoin_common_a_SOURCES"
(https://github.com/bitcoin/bitcoin/pull/30427#pullrequestreview-2171804272)
ACK e8c3b7172c33929e4e5bf6059da2d25a4ea8779c

💬 Sjors commented on pull request "Stratum v2 Template Provider (take 3)":
(https://github.com/bitcoin/bitcoin/pull/29432#issuecomment-2222894099)
Addressed linter issues and fixed deadlock regression in 02f1f86b6b6d46d8ab5e17d5d824b0c1bba5d789 -> 00201802a19124cc8eb671b7f9311326f99f999d.

💬 Eunovo commented on pull request "Tr partial descriptors":
(https://github.com/bitcoin/bitcoin/pull/30243#issuecomment-2222925877)
> You can squash some commits.

Thanks @brunoerg. Done!