💬 stratospher commented on pull request "test: bump mocktime only after node has received and sent bytes":
(https://github.com/bitcoin/bitcoin/pull/30468#discussion_r1680951432)
I couldn't reproduce it locally.

since peer timeout is `3`, `InactivityCheck()`(and hence disconnection) will happen when our time is` > 3`. so disconnection will only happen during the next `bumpmocktime(1)` I think.

(Also see https://github.com/bitcoin/bitcoin/pull/30420#discussion_r1680773490)

💬 maflcko commented on issue "Feature Request: Broadcast Pool":
(https://github.com/bitcoin/bitcoin/issues/30471#issuecomment-2233185138)
> making this existing functionality available to all transactions broadcasted through the node and not just those created by a local wallet.

Would it not be possible to implement this by creating a wallet called `broadcast_pool` and then write simple wrapper functions to implement the broadcast pool features?

> Future-dated transactions are also included in views on the mempool, but are marked as unbroadcast: true in mempool RPC responses

I don't think it is possible to include somethi
...

💬 Sjors commented on pull request "multiprocess: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#discussion_r1680953879)
8a20be71f07cfe0129107cb8effb2490b79639bf: `#include <logging.h>`

💬 hodlinator commented on pull request "refactor: Make uint256S(const char*) and ParseHex(const char*) consteval":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1680955841)
Thanks for taking a closer look!

I want to enforce `consteval` for existing string literals. Ideally the `string_view` overload should be `constexpr` but from my research on MSVC assembly output in the PR summary it seems to handle lookup tables poorly. One possibility would be to remove the lookup table and use `if (c >= '0' && c <= '9')`, sacrificing some performance, but we don't often parse ASCII hex strings in runtime anyway, right?

The main purpose of overloading `uint256S()` with a
...

💬 maflcko commented on pull request "test: bump mocktime only after node has received and sent bytes":
(https://github.com/bitcoin/bitcoin/pull/30468#discussion_r1680958506)
Ah, I see the check is `<`, not `<=`.

However, what is the purpose of two immediately following `bumpmocktime` on the same node? Wouldn't it be easier to just combine them into one (and use the sum of their arguments)?

💬 maflcko commented on pull request "test: bump mocktime only after node has received and sent bytes":
(https://github.com/bitcoin/bitcoin/pull/30468#issuecomment-2233196862)
review ACK e6f4614ea57dbd3d8690fdbed55ecf2612619070

Looks good, feel free to ignore the nit.

💬 hodlinator commented on pull request "refactor: Make uint256S(const char*) and ParseHex(const char*) consteval":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1680963050)
My first stab at this was much more strict but when I added tests I went for full compatibility with the runtime overload. I'm certainly open to making it less permissive though.

💬 hodlinator commented on pull request "refactor: Make uint256S(const char*) and ParseHex(const char*) consteval":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1680963523)
Same (https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1680963050)

💬 stratospher commented on pull request "test: bump mocktime only after node has received and sent bytes":
(https://github.com/bitcoin/bitcoin/pull/30468#discussion_r1680964397)
true, done.

💬 maflcko commented on pull request "refactor: Make uint256S(const char*) and ParseHex(const char*) consteval":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1680966315)
> `uint256(uint64_t,uint64_t,uint64_t,uint64_t)` constructor. That way it would be the compiler parsing hexadecimal integer literals directly.

Not sure. That'd make it impossible to grep for a (let's say) block hash. Also, it would be harder to copy-paste one, if the developer has to manually split it into 4 parts and add `0x` prefixes. Finally, truncation checks can't be done, see https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1680422462

> was to avoid changing all the call-sit
...

💬 maflcko commented on pull request "refactor: Make uint256S(const char*) and ParseHex(const char*) consteval":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1680967973)
> I want to enforce `consteval` for existing string literals. Ideally the `string_view` overload should be `constexpr` but from my research on MSVC assembly output

I think `string_view` can be used in a consteval context, no?

💬 maflcko commented on pull request "test: bump mocktime only after node has received and sent bytes":
(https://github.com/bitcoin/bitcoin/pull/30468#issuecomment-2233211540)
reACK c322bddd08ed1f1f7f0c39768b659dd62d5e2dd5

💬 hodlinator commented on pull request "refactor: Make uint256S(const char*) and ParseHex(const char*) consteval":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1680976118)
There is no `uint160S()` outside of **uint256_tests.cpp** and only `uint256` and `uint160` use `base_blob` so it felt alright having it here. If we abandon the `uint256S()`-overload approach I agree to change this.

📝 Igor258 opened a pull request: "btc"
(https://github.com/bitcoin/bitcoin/pull/30472)
<!--
*** Please remove the following help text before submitting: ***

Pull requests without a rationale and clear improvement may be closed
immediately.

GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->

<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:

* Any test improvements or new tests that improv
...

✅ fanquake closed a pull request: "btc"
(https://github.com/bitcoin/bitcoin/pull/30472)

📝 fanquake locked a pull request: "btc"
(https://github.com/bitcoin/bitcoin/pull/30472)
<!--
*** Please remove the following help text before submitting: ***

Pull requests without a rationale and clear improvement may be closed
immediately.

GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->

<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:

* Any test improvements or new tests that improv
...

💬 hodlinator commented on pull request "refactor: Make uint256S(const char*) and ParseHex(const char*) consteval":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1680983277)
Generally agree https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1680963050

(Some call-sites do use "0x"-prefix now that I think of it, while others do not, but aligning that would probably be an easy diff to accept).

💬 hodlinator commented on pull request "refactor: Make uint256S(const char*) and ParseHex(const char*) consteval":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1680986937)
> > I want to enforce `consteval` for existing string literals. Ideally the `string_view` overload should be `constexpr` but from my research on MSVC assembly output
>
> I think `string_view` can be used in a consteval context, no?

Yes, it's the rest of that paragraph that poses some concern.

💬 fanquake commented on pull request "refactor: Make uint256S(const char*) and ParseHex(const char*) consteval":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1680995968)
> Ideally the string_view overload should be constexpr but from my research on MSVC assembly output

Please don't be overly concerned about MSVC. We've had issues with it in the past, where it's failed to optimize code properly, or we've had to write workarounds for it, when code was otherwise fine in Clang or GCC, i.e (https://github.com/bitcoin/bitcoin/pull/28657#discussion_r1360780446). We don't use it as a release compiler (and never will), so if it's failing to do X, that isn't necessaril
...

✅ fanquake closed an issue: "getaddressinfo: complains missing `isscript` when called on unknown witness version"
(https://github.com/bitcoin/bitcoin/issues/30456)