💬 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!

💬 Eunovo commented on pull request "locks: introduce mutex for tx download, flush rejection filters on UpdatedBlockTip":
(https://github.com/bitcoin/bitcoin/pull/30111#discussion_r1674013463)
@glozow Thanks for the clarification. I see it now. Any reason why we don't fire the `ActiveTipChange` here instead?

💬 glozow commented on pull request "locks: introduce mutex for tx download, flush rejection filters on UpdatedBlockTip":
(https://github.com/bitcoin/bitcoin/pull/30111#discussion_r1674034705)
What do you mean by "here"?

💬 glozow commented on pull request "BlockAssembler: return selected packages vsize and feerate":
(https://github.com/bitcoin/bitcoin/pull/30391#discussion_r1672095708)
probably `FeeFrac` would be better? `CFeeRate` loses precision and can be inferred from the fee and size

💬 Eunovo commented on pull request "locks: introduce mutex for tx download, flush rejection filters on UpdatedBlockTip":
(https://github.com/bitcoin/bitcoin/pull/30111#discussion_r1674043326)
By "here", I mean in https://github.com/bitcoin/bitcoin/blob/9b480f7a25a737c9c4ebc33401e94d66c2da9ec3/src/validation.cpp#L3505-L3508

🤔 hebasto reviewed a pull request: "contrib: simplify `test-security-check`"
(https://github.com/bitcoin/bitcoin/pull/30423#pullrequestreview-2171924002)
Concept ACK.

nit: Spaces after commas and around `+` can used more consistently for better readability.

💬 hebasto commented on pull request "contrib: simplify `test-security-check`":
(https://github.com/bitcoin/bitcoin/pull/30423#discussion_r1674044249)
This check fails:
```
FAIL: test_PE (__main__.TestSecurityChecks)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/distsrc-base/distsrc-ea096063c384-x86_64-w64-mingw32/./contrib/devtools/test-security-check.py", line 88, in test_PE
self.assertEqual(call_security_check(cc, source, executable, pass_flags + ['-Wl,--disable-reloc-section']), (1, executable+': failed RELOC_SECTION'))
AssertionError: Tuples differ: (1, 'test1
...

💬 hebasto commented on pull request "contrib: simplify `test-security-check`":
(https://github.com/bitcoin/bitcoin/pull/30423#discussion_r1674030590)
Is the `-pie` flag supported here?

From the `ld` linker [docs](https://sourceware.org/binutils/docs/ld.html#index-_002dpie):
> This is currently only supported on ELF platforms.