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

💬 fanquake commented on pull request "contrib: simplify `test-security-check`":
(https://github.com/bitcoin/bitcoin/pull/30423#discussion_r1674050332)
Those look like the wrong docs:
```bash
x86_64-w64-mingw32-ld --help | grep pie
-pie, --pic-executable Create a position independent executable
-no-pie Create a position dependent executable (default)
```

💬 pinheadmz commented on pull request "system: use %LOCALAPPDATA% as default datadir on windows":
(https://github.com/bitcoin/bitcoin/pull/27064#discussion_r1674079452)
Ok thanks again, I'll make sure it gets corrected at that time

💬 fanquake commented on pull request "contrib: simplify `test-security-check`":
(https://github.com/bitcoin/bitcoin/pull/30423#discussion_r1674084480)
This has turned up what looks like a bug in LIEF: https://github.com/lief-project/LIEF/issues/1076. Will re-enable the test later.

🤔 glozow reviewed a pull request: "#28984 package rbf followups"
(https://github.com/bitcoin/bitcoin/pull/30295#pullrequestreview-2172049102)
ACK 3f00aae1409