💬 maflcko commented on pull request "util: Use consteval checked format string in FatalErrorf":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1752491985)
> Sorry for not realizing and raising this in an earlier review.

No worries. It was my mistake for pushing the wrong diff.


My initial patch from July only had very basic checking and accepted any format specifier (like tinyformat). When I added support for positional args a few days ago (https://github.com/bitcoin/bitcoin/pull/30546#issuecomment-2333713098), I also tried to validate format specifiers itself.

However, given that tinyformat accepts any specifier, and given that incorrec
...

💬 maflcko commented on pull request "util: Use consteval checked format string in FatalErrorf":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1752494409)
Thanks for a diff that compiles. However, I think that making implementation details verbose to use is a feature to avoid developers from accidentally using them in real code. The verbosity in tests is a bit annoying, but seems acceptable to me.

I'll leave this as-is for now.

💬 maflcko commented on pull request "util: Use consteval checked format string in FatalErrorf":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1752497709)
Thanks! I think it was wrong for me to try to re-implement tinyformat at compile time and drop the features that aren't currently needed. I've reverted the pull to an earlier state, so that any tinyformat feature continues to work, just like before. (C.f. https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1752491985)

💬 maflcko commented on pull request "util: Use consteval checked format string in FatalErrorf":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1752498486)
Thanks, and good catch! I've reverted this, see https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1752491985

👍 TheCharlatan approved a pull request: "Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange, drop CRPCSignals & g_best_block"
(https://github.com/bitcoin/bitcoin/pull/30409#pullrequestreview-2293537994)
Re-ACK 966027bdda53d150321af7db48b57f9756c54e68

📝 hebasto opened a pull request: "build: Skip secp256k1 ctime tests when tests are not being built"
(https://github.com/bitcoin/bitcoin/pull/30865)
Fixes https://github.com/bitcoin/bitcoin/pull/30791#issuecomment-2340860619:
> Building with a fuzz engine fails, because the ctime tests are auto-detected in cmake, based on whether or not valgrind-devel is installed or not.

💬 hebasto commented on pull request "build: Use correct variable name":
(https://github.com/bitcoin/bitcoin/pull/30791#issuecomment-2341839951)
> This uncovers a bug in the build system: Building with a fuzz engine fails, because the ctime tests are auto-detected in cmake, based on whether or not valgrind-devel is installed or not.

Fixed in https://github.com/bitcoin/bitcoin/pull/30865.

💬 mzumsande commented on issue "28.0 RC Testing Guide Feedback":
(https://github.com/bitcoin/bitcoin/issues/30854#issuecomment-2341859266)
> How about adding an instruction for testing assumeutxo mainnet parameters?

Not related to the testing guide, but it could also be mentioned in the release notes that assumeutxo mainnet params are available now.

👍 TheCharlatan approved a pull request: "multiprocess: Add IPC wrapper for Mining interface"
(https://github.com/bitcoin/bitcoin/pull/30510#pullrequestreview-2293613874)
ACK 9c98c42a0166e9e201f6e9d32a0692fad5a185f0

👍 ryanofsky approved a pull request: "Remove unsafe uint256S() and test-only uint160S()"
(https://github.com/bitcoin/bitcoin/pull/30773#pullrequestreview-2293623641)
Code review ACK 43cd83b0c7ba436b8ffc83d8a65e935714dffe70. Only code change is a small refactoring which looks good. The rest of the PR is all test changes, which I only lightly reviewed, but seem to be positive and do what's described

🚀 ryanofsky merged a pull request: "Remove unsafe uint256S() and test-only uint160S()"
(https://github.com/bitcoin/bitcoin/pull/30773)

📝 achow101 opened a pull request: "descriptor: Add proper Clone function to miniscript::Node"
(https://github.com/bitcoin/bitcoin/pull/30866)
Multipath descriptors requires performing a deep copy, so a Clone function that does that is added to miniscript::Node instead of the current shallow copy.

Fixes #30864

💬 achow101 commented on issue "fuzz: `scriptpubkeyman`: heap-buffer-overflow miniscript.cpp in CScript BuildScript":
(https://github.com/bitcoin/bitcoin/issues/30864#issuecomment-2341894445)
#30866

💬 achow101 commented on pull request "test: Add explicit onion bind to p2p_permissions":
(https://github.com/bitcoin/bitcoin/pull/30805#discussion_r1752653030)
Done

💬 maflcko commented on pull request "build: Skip secp256k1 ctime tests when tests are not being built":
(https://github.com/bitcoin/bitcoin/pull/30865#issuecomment-2341907938)
review ACK 8fb81c21cfaaee4ea68e5023c31d58b01f53fa4f

👍 tdb3 approved a pull request: "test: Add explicit onion bind to p2p_permissions"
(https://github.com/bitcoin/bitcoin/pull/30805#pullrequestreview-2293665953)
ACK a1e737ef089d407256428ea29be20f46bbea71fe

👍 ryanofsky approved a pull request: "refactor: Allow `CScript`'s `operator<<` to accept spans, not just vectors"
(https://github.com/bitcoin/bitcoin/pull/30765#pullrequestreview-2293686068)
Code review ACK 48b67d8294eb62c5371e7dbc30b7cf811fb7e51c. This looks great! PR is much simpler now, and it is nice to see the GCC workaround expanded and documented so well.

Would suggest a few changes, implemented in diff below:

- Replacing `PushDataSize` `size_t` parameter with `uint32_t` parameter to document fact that it can't accept larger sizes, and to call `WriteLE32` without a type conversion.
- Replacing `PushData` `const value_type* data, size_t size` parameters with `std::span<
...

💬 furszy commented on pull request "Fix peers abruptly disconnecting from AssumeUTXO nodes during IBD":
(https://github.com/bitcoin/bitcoin/pull/30807#discussion_r1752677734)
Done. Updated.

💬 furszy commented on pull request "Fix peers abruptly disconnecting from AssumeUTXO nodes during IBD":
(https://github.com/bitcoin/bitcoin/pull/30807#discussion_r1752677951)
Done. Updated.

💬 furszy commented on pull request "Fix peers abruptly disconnecting from AssumeUTXO nodes during IBD":
(https://github.com/bitcoin/bitcoin/pull/30807#discussion_r1752678000)
Done. Updated.