💬 darosior commented on pull request "test: Add mockable steady clock, tests for PCP and NATPMP implementations":
(https://github.com/bitcoin/bitcoin/pull/31022#issuecomment-2464948588)
Concept ACK

👍 maflcko approved a pull request: "tinyformat: Add compile-time checking for literal format strings"
(https://github.com/bitcoin/bitcoin/pull/31174#pullrequestreview-2423906139)
left some nit ideas for more tests, but this is good either way.

review ACK ecc5cb9a89c6b001df839675b23d8fc1f7ac69ba 🕯

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+kr
...

💬 maflcko commented on pull request "tinyformat: Add compile-time checking for literal format strings":
(https://github.com/bitcoin/bitcoin/pull/31174#discussion_r1834499010)
Could add new failing cases here as well?


```
FailFmtWithError<2>("%2$*$d", err_0_pos);
FailFmtWithError<2>("%2$*0$d", err_0_pos);
FailFmtWithError<3>("%3$*2$.*$f", err_0_pos);
FailFmtWithError<3>("%3$*2$.*0$f", err_0_pos);
```

💬 maflcko commented on pull request "tinyformat: Add compile-time checking for literal format strings":
(https://github.com/bitcoin/bitcoin/pull/31174#discussion_r1834505494)
Same (below):

```
FailFmtWithError<1>("%*c", err_num);

💬 maflcko commented on pull request "tinyformat: Add compile-time checking for literal format strings":
(https://github.com/bitcoin/bitcoin/pull/31174#discussion_r1834504076)
Same:

```
FailFmtWithError<2>("%2$*d", err_mix);
FailFmtWithError<2>("%*2$d", err_mix);
FailFmtWithError<2>("%.*3$d", err_mix);
FailFmtWithError<2>("%2$.*d", err_mix);

💬 Sjors commented on pull request "init: warn, don't error, when '-upnp' is set":
(https://github.com/bitcoin/bitcoin/pull/31198#issuecomment-2464977943)
cc @ryanofsky who recently refactored a lot of the settings code.

💬 maflcko commented on pull request "test: Shut down framework cleanly on RPC connection failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1834551086)
I still don't know if the current docs are correct and useful.

Why would `ETIMEDOUT` only happen on warmup? Why is it even relevant to mention anything here?

I think it would be clearer to just mention that the three are treated equal to "-342 Service unavailable", which is already explained above?

💬 Sjors commented on issue "Mining Interface doesn't allow for Bitcoin Core to create blocks when it wants":
(https://github.com/bitcoin/bitcoin/issues/31109#issuecomment-2465021413)
> because the `BlockTemplate` object provides information about the last block sent to the client.

Good point, the lack of such a reference has been a source of headaches.

I plan to look into this next week.

💬 Sjors commented on pull request "Prune mining interface":
(https://github.com/bitcoin/bitcoin/pull/31196#issuecomment-2465040293)
@dergoegge I made a note to re-introduce a verification method for blocks. IIUC the use case would be to verify externally generated blocks, similar to how `proposal` works over RPC - but faster because it avoids RPC overhead.

I'd rather design that method from scratch (dropping the current implementation). E.g. it should provide useful feedback if our tip doesn't match, maybe even have an option to reorg in that case, etc. This is not needed for Stratum v2, so it's lower on my list of priori
...

💬 Sjors commented on pull request "Add waitFeesChanged() to Mining interface":
(https://github.com/bitcoin/bitcoin/pull/31003#issuecomment-2465046092)
Marking draft because I might drop this method in favor of the approach suggested in https://github.com/bitcoin/bitcoin/issues/31109#issuecomment-2450648747

📝 Sjors converted_to_draft a pull request: "Add waitFeesChanged() to Mining interface"
(https://github.com/bitcoin/bitcoin/pull/31003)
The Stratum v2 protocol allows pushing out templates as fees in the mempool increase. This interface lets us know when it's time to do so.

I split the implementation into two parts because I'm not sure if we should include the second commit now, or wait until cluster mempool #30289 is merged.

The first commit contains a mock implementation very similiar to how longpolling in `getblocktemplate` works. It calls `getTransactionsUpdated` once per second. This returns `true` anytime a transacti
...

💬 Sjors commented on pull request "refactor: mining interface 30955 followups":
(https://github.com/bitcoin/bitcoin/pull/31197#discussion_r1834587201)
Will do if I need to retouch.

💬 Sjors commented on pull request "BlockAssembler: return selected packages virtual size and fee":
(https://github.com/bitcoin/bitcoin/pull/30391#issuecomment-2465065423)
Just a comment about how this interacts with the Mining interface changes I'm working on.

This PR adds a new field to CBlockTemplate. The Mining interface contains BlockTemplate:

https://github.com/bitcoin/bitcoin/blob/018e5fcc462caebb25cf5d3eb6a19dc2787466c8/src/interfaces/mining.h#L31-L40

It has a method to return the `CBlockHeader`. I wonder if adding a field to CBlockTemplate would cause a breaking change for clients connecting via IPC. cc @ryanofsky?

In any case it won't matter
...

💬 mzumsande commented on pull request "fuzz: fix bad alloc in connman target":
(https://github.com/bitcoin/bitcoin/pull/31235#discussion_r1834609945)
> Do you remember the bug number? "The reason max_pct exists is not to reveal the entire addrman", but then passing even == 100 is a bug?

GetAddr answers have been capped by percentage at least since addrman was introduced (5fee401fe14aa6459428a26a82f764db70a6a0b9), just that `ADDRMAN_GETADDR_MAX_PCT=23` was hardcoded in the past. It's kind of still that way (`ADDRMAN_GETADDR_MAX_PCT` is used as an arg in net_processing, the rpc code path doesn't use `max_pct`.), so in current master nothing
...

📝 fanquake locked a pull request: "doc: Add missing 'blank=true' option in offline-signing-tutorial.md"
(https://github.com/bitcoin/bitcoin/pull/31236)
### **Issue:**

The text mentions that the `createwallet` command should use the options `disable_private_keys=true, blank=true`, but the provided command only includes `disable_private_keys=true`, missing the `blank=true` option.

---

### **Details:**

**Original Text:**

> This is achieved by using the `createwallet` options: `disable_private_keys=true, blank=true`.

**Original Command:**

```sh
[online]$ ./build/src/bitcoin-cli -signet -named createwallet \
wall
...

🤔 jonatack reviewed a pull request: "test: addrman: tried 3 times and never a success so `isTerrible=true`"
(https://github.com/bitcoin/bitcoin/pull/30445#pullrequestreview-2424312395)
ACK d6fabbe2d40b53f6fa35d6b0191f3d73999d6b41

Modulo mentioning and clarifying the time aspect in the test (see comment below) and the PR description.

💬 jonatack commented on pull request "test: addrman: tried 3 times and never a success so `isTerrible=true`":
(https://github.com/bitcoin/bitcoin/pull/30445#discussion_r1834706023)
The time setting here seems to be critical and could be commented. It looks like the threshold between success and failure is between `60s` and `61s`. Is that the case for you as well, and could you explain here why?

💬 achow101 commented on pull request "test: Add combinerawtransaction test to rpc_createmultisig":
(https://github.com/bitcoin/bitcoin/pull/31249#issuecomment-2465262582)
> Forgot to move [ab98e6f](https://github.com/bitcoin/bitcoin/commit/ab98e6fd03970d6b5a593674c84e762a47b90ea6) as well?

Pulled those in too

💬 achow101 commented on pull request "descriptor: Add proper Clone function to miniscript::Node":
(https://github.com/bitcoin/bitcoin/pull/30866#discussion_r1834730377)
Removed

💬 maflcko commented on pull request "test: Add combinerawtransaction test to rpc_createmultisig":
(https://github.com/bitcoin/bitcoin/pull/31249#issuecomment-2465289666)
re-ACK 83fab3212c91d91fc5502f940c901a07772ff747