📝 fanquake locked a pull request: "btc"
(https://github.com/bitcoin/bitcoin/pull/30470)
<!--
*** 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
...

👍 stickies-v approved a pull request: "qa: Functional test improvements"
(https://github.com/bitcoin/bitcoin/pull/30463#pullrequestreview-2182628847)
ACK a8e3af1a82dd584a1cc3ffbe587e66889f72e3c7

I'm able to run individual tests from an out-of-source build with the instructions provided. I tried running a bunch of other tests than `wallet_disable` too, including all the ones that reference `__file__` and those seem to work fine too.

💬 vasild commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1680885121)
I admit I am somewhat confused.

Do we assume that for IPv6, address translation is never done (e.g. https://www.rfc-editor.org/rfc/rfc6296) and that for IPv6 we only need to pinhole the firewall?

> For IPv6 we know what internal address we want, and what external address we want

You mean that both internal and external are the same and we get that from `GetLocalAddresses()`?

👍 tdb3 approved a pull request: "doc: getaddressinfo[isscript] is optional"
(https://github.com/bitcoin/bitcoin/pull/30457#pullrequestreview-2182648657)
ACK fa6390df205513319f28e35e3e17c40ecaa7d731

💬 maflcko commented on pull request "qa: Functional test improvements":
(https://github.com/bitcoin/bitcoin/pull/30463#discussion_r1680899761)
Maybe, but it would be good to clarify what of the previous conversation is relevant.

Are you saying that this is needed for Windows and can not be tested on Unix?

💬 maflcko commented on pull request "index: Fix coinstats overflow and introduce index versioning":
(https://github.com/bitcoin/bitcoin/pull/30469#discussion_r1680902695)
:eye:

💬 maflcko commented on pull request "test: Non-Shy version sender":
(https://github.com/bitcoin/bitcoin/pull/30453#discussion_r1680905532)
Thanks, taken!

💬 maflcko commented on pull request "test: bump mocktime only after node has received and sent bytes":
(https://github.com/bitcoin/bitcoin/pull/30468#discussion_r1680907742)
This will be racy, no?

The peer timeout is `3`, which is also the bump, so the peer may be disconnected at any time, leading to a test failure if it is disconnected at the wrong time.

Something like this should fail the test, no?

```suggestion
node0.bumpmocktime(3); time.sleep(1);
```

👋 maflcko's pull request is ready for review: "rest: Reject negative outpoint index early in getutxos parsing"
(https://github.com/bitcoin/bitcoin/pull/30444)

💬 maflcko commented on pull request "rest: Reject negative outpoint index early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30444#issuecomment-2233122996)
Pushed a commit to fix a harmless `unsigned-integer-overflow` warning.

@hodlinator Happy to review your suggestions to other functions in a separate pull request, if you create one. But I'll try to keep this one focussed for now.

⚠️ craigraw opened an issue: "Feature Request: Broadcast Pool"
(https://github.com/bitcoin/bitcoin/issues/30471)
### Please describe the feature you'd like to see added.

This is a feature request for a broadcast pool. A broadcast pool is a cache local to the node which contains transactions which have been initially broadcast from that node, either from a local wallet or via an RPC like `sendrawtransaction`. Broadcast transactions are retained in this cache until they are included in a block, or expire after the configured `mempoolexpiry` period. The transactions in the broadcast pool are included in view
...

💬 hebasto commented on pull request "depends: build zeromq with CMake":
(https://github.com/bitcoin/bitcoin/pull/29723#issuecomment-2233129404)
> > From [zeromq/libzmq#4667 (comment)](https://github.com/zeromq/libzmq/pull/4667#issue-2197755398):
>
> Can you elaborate / suggest something concrete?

Please see https://github.com/zeromq/libzmq/pull/4706.

👍 hodlinator approved a pull request: "rest: Reject negative outpoint index early in getutxos parsing"
(https://github.com/bitcoin/bitcoin/pull/30444#pullrequestreview-2182729770)
ut-ACK fac932bf93d9bd8cb7bef93f04ecf48ea2ccf1d1

Nice that the added tests managed to trigger the sanitizer! :+1:

💬 vasild commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1680951137)
Would all-zeros work as well for IPv6 / pinholing? If yes, then this can probably be simplified to always pass all-zeros.

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