💬 sipa commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1773194231)
I think it makes sense to keep the two separate. Just because two specifications ("Bitcoin ADDR message encoding" and "RFC6887") happen to be largely the same doesn't mean they should be treated as a single thing, and in that case it seems cleaner to keep the implementations separate too.

💬 l0rinc commented on pull request "doc: correct the zmq automatic build info":
(https://github.com/bitcoin/bitcoin/pull/30946#discussion_r1773209483)
I also noticed the same.
this line should be executable code, could we either remove `(other options)` or add it as a comment instead?

💬 sipa commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#issuecomment-2371111205)
Tested on my Spectrum-provided cable modem (through NATPMP fallback), and am getting inbound connections.

💬 marcofleon commented on issue "Test p2p_headers_presync fuzz target on macOS/Windows":
(https://github.com/bitcoin/bitcoin/issues/30950#issuecomment-2371157459)
Looking into this.

📝 hodlinator opened a pull request: "DO NOT MERGE: Windows bitcoind stall debugging"
(https://github.com/bitcoin/bitcoin/pull/30956)
This PR is meant to help investigate #30390 (https://github.com/bitcoin/bitcoin/issues/30390#issuecomment-2326628987).

Switches Windows CI to produce *RelWithDebInfo* binaries and adds code that uses *ProcDump* to generate a dump file of the *bitcoind* process which may help explain where it's gotten stuck.

Unresolved:
- Whether the CI change is remotely correct.
- How to get access to the generated .DMP and .PDB files.

📝 hodlinator converted_to_draft a pull request: "DO NOT MERGE: Windows bitcoind stall debugging"
(https://github.com/bitcoin/bitcoin/pull/30956)
This PR is meant to help investigate #30390 (https://github.com/bitcoin/bitcoin/issues/30390#issuecomment-2326628987).

Switches Windows CI to produce *RelWithDebInfo* binaries and adds code that uses *ProcDump* to generate a dump file of the *bitcoind* process which may help explain where it's gotten stuck.

Unresolved:
- Whether the CI change is remotely correct.
- How to get access to the generated .DMP and .PDB files.

🤔 l0rinc reviewed a pull request: "refactor: Implement missing error checking for ArgsManager flags"
(https://github.com/bitcoin/bitcoin/pull/16545#pullrequestreview-2325050760)
> C++ developers should be more comfortable with type declarations like std::variant<bool, std::string> than equivalent declarations with flags like ALLOW_BOOL | ALLOW_STRING

`std::variant` is basically a poor-man's-try-monad, until we can use `std::expected` in C++23.

----

I started reviewing the PR, but rereading your comment I'm not sure whether you want to modify this part twice - because I'm all for a more functional approach instead of playing with flags (did a similar refactor in
...

💬 l0rinc commented on pull request "refactor: Implement missing error checking for ArgsManager flags":
(https://github.com/bitcoin/bitcoin/pull/16545#discussion_r1773221823)
nit: you're using both `can not` and `cannot` in the errors

💬 l0rinc commented on pull request "refactor: Implement missing error checking for ArgsManager flags":
(https://github.com/bitcoin/bitcoin/pull/16545#discussion_r1773293766)
nit: I haven't seen this style used here, it's meant to keep the note's formatting, right? (edit, I see @hodlinator already noticed the same, but now it's `cmake --build build --target docs`)
It seems to me we can achieve the same by removing the line completely:
<img width="356" alt="image" src="https://github.com/user-attachments/assets/99080cad-1be4-4524-bd23-14d093762dde">

💬 l0rinc commented on pull request "refactor: Implement missing error checking for ArgsManager flags":
(https://github.com/bitcoin/bitcoin/pull/16545#discussion_r1773291395)
```suggestion
* @return parsed settings value if it is valid, otherwise `nullopt` accompanied
```

<img width="645" alt="image" src="https://github.com/user-attachments/assets/f11f67dd-04ae-492d-b053-07471e6e4ecc">

⚠️ brunoerg opened an issue: "docs: "Fuzzing the Bitcoin Core P2P layer using Honggfuzz NetDriver" is outdated"
(https://github.com/bitcoin/bitcoin/issues/30957)
Similar to #28019. The following instruction is outdated and doesn't work:
```diff
$ git apply << "EOF"
diff --git a/src/compat/compat.h b/src/compat/compat.h
index 8195bceaec..cce2b31ff0 100644
--- a/src/compat/compat.h
+++ b/src/compat/compat.h
@@ -90,8 +90,12 @@ typedef char* sockopt_arg_type;
// building with a binutils < 2.36 is subject to this ld bug.
#define MAIN_FUNCTION __declspec(dllexport) int main(int argc, char* argv[])
#else
+#ifdef HFND_FUZZING_ENTRY_FUNCTION_CXX
+#
...

⚠️ fanquake opened an issue: "`invalidateblock` during background validation"
(https://github.com/bitcoin/bitcoin/issues/30958)
If you call `invalidateblock` (on the tip) during background validation, the node will currently do the following:
#### bitcoin-cli invalidateblock 00000000000000000000f5752e9599f03a7b3a3e2356af91faaf74e41fdf58fe
```bash
2024-09-24T13:01:15Z [background validation] UpdateTip: new best=0000000000000000008dd2857255b2e5fc0c3955740c74dac6bafd0de09c344e height=482000 version=0x20000000 log2_work=86.991735 tx=249395394 date='2017-08-25T19:52:51Z' progress=0.230269 cache=238.7MiB(1748399txo)
2024-0
...

🤔 maflcko reviewed a pull request: "Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange, drop CRPCSignals & g_best_block"
(https://github.com/bitcoin/bitcoin/pull/30409#pullrequestreview-2324834610)
review ACK 7942951e3fcc58f7db0059546d03be9ea243f1db 🛋

<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+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK 7942951e3fcc
...

💬 maflcko commented on pull request "Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange, drop CRPCSignals & g_best_block":
(https://github.com/bitcoin/bitcoin/pull/30409#discussion_r1773095740)
nit in 7eccdaf16081d6f624c4dc21df75b0474e049d2b: Any reason to use the throwing function? The internal error isn't caught anywhere, IIUC. So, the program will terminate either way. By using `*Assert(AnyPtr(context))` here instead, at least a usable error message is printed.

<!--
For reference:

```
2024-09-24T10:38:06Z opencon thread exit
terminate called after throwing an instance of 'UniValue'
Aborted (core dumped)

💬 maflcko commented on pull request "Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange, drop CRPCSignals & g_best_block":
(https://github.com/bitcoin/bitcoin/pull/30409#discussion_r1773215233)
b94b27cf05c709674117e308e441a8d1efddcd0a: Can you explain the meaning of "overflow" in the context of `double`?

💬 pablomartin4btc commented on issue "show error "could not sign any more inputs" when sign PSBT for multisig":
(https://github.com/bitcoin/bitcoin/issues/30177#issuecomment-2371290717)
This is being addressed in the GUI repository at https://github.com/bitcoin-core/gui/pull/832.

💬 maflcko commented on pull request "rpc: show P2(W)SH redeemScript in getrawtransaction #27637":
(https://github.com/bitcoin/bitcoin/pull/27638#issuecomment-2371310988)
You'll have to put in some effort to create a pull request that is reviewable. It is fine to have a draft pull request, while you work on it, or ask specific questions about the implementation. However, it is unlikely that someone is going to review an incomplete draft pull request that needs rebase.

💬 Sjors commented on pull request "Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange, drop CRPCSignals & g_best_block":
(https://github.com/bitcoin/bitcoin/pull/30409#discussion_r1773416406)
I probably just copied similar code from another place :-)

Can make a followup, though `*Assert(AnyPtr(context))` looks pretty scary.

💬 Sjors commented on pull request "Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange, drop CRPCSignals & g_best_block":
(https://github.com/bitcoin/bitcoin/pull/30409#discussion_r1773420568)
It was in response to: https://github.com/bitcoin/bitcoin/pull/30409#discussion_r1713879417

💬 KristijanSajenko commented on pull request "Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange, drop CRPCSignals & g_best_block":
(https://github.com/bitcoin/bitcoin/pull/30409#issuecomment-2371390776)
Is it workin

💬 maflcko commented on pull request "Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange, drop CRPCSignals & g_best_block":
(https://github.com/bitcoin/bitcoin/pull/30409#discussion_r1773447523)
You probably copied it from an RPC method implementation. There, it would be the right choice, because the http threads catch exceptions and then continue with the next RPC call.

However, here no exceptions are caught (and doing so wouldn't make sense, probably). The program will terminate either way, for example:

```
2024-09-24T10:38:06Z opencon thread exit
terminate called after throwing an instance of 'UniValue'
Aborted (core dumped)
```

However, by using `Assert`/`assert`, at le
...