📝 laanwj opened a pull request: "init: Handle dropped UPnP support more gracefully"
(https://github.com/bitcoin/bitcoin/pull/31916)
Closes bitcoin-core/gui#843.

In that issue it was brought up that users likely don't care what kind of port forwarding is used, and that the setting is opportunistic anyway, so instead of showing an extensive warning, we can simply "upgrade" from UPNP to NAT-PMP+PCP.

- Change the logic for removed runtime setting `-upnp` to set `-natpmp` instead, and log a message.

- Also remove any lingering `upnp` from `settings.json` and replace it with `natpmp`, when it makes sense (for the UI):

...

💬 achow101 commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1963881147)
Removed the `else`.

👍 theStack approved a pull request: "cmake: Set top-level target output locations"
(https://github.com/bitcoin/bitcoin/pull/31161#pullrequestreview-2630429098)
Lightly tested re-ACK f7d8a44ce42cce0b3010a7f7d14ec180e959f3e4

(retested as described in https://github.com/bitcoin/bitcoin/pull/31161#pullrequestreview-2578563294 before)

💬 pinheadmz commented on pull request "guix: Notarize MacOS app bundle and codesign all MacOS and Windows binaries":
(https://github.com/bitcoin/bitcoin/pull/31407#issuecomment-2671982208)
Can confirm behavior described by @Sjors. Seems like apple is confusing itself?

<img src="https://github.com/user-attachments/assets/1774cca4-92e1-4c08-a8cc-34fe195c2cc0" width="300">

but binary is signed:

```
--> codesign -vd --verbose=4 /Users/matthewzipkin/Desktop/bitcoin-e181bda061ca/bin/bitcoind
Executable=/Users/matthewzipkin/Desktop/bitcoin-e181bda061ca/bin/bitcoind
Identifier=bitcoind
Format=Mach-O thin (arm64)
CodeDirectory v=20500 size=23284 flags=0x10000(runtime) hashes
...

💬 maflcko commented on pull request "init: Handle dropped UPnP support more gracefully":
(https://github.com/bitcoin/bitcoin/pull/31916#discussion_r1963919027)
Is this the right check? IIUC `IsArgSet` will return true even when the value is negated (`-noupnp`)?

👍 theuni approved a pull request: "cmake: Make implicit `libbitcoinkernel` dependencies explicit"
(https://github.com/bitcoin/bitcoin/pull/31884#pullrequestreview-2630455667)
utACK 3b42e05aa9e38ba00d52b2338375b4caf032f041

Thanks for the fix!

💬 mprenditore commented on pull request "Added rescan option for import descriptors":
(https://github.com/bitcoin/bitcoin/pull/31668#discussion_r1963931431)
I do agree to add `never` to the list of timestamp values to handle this case.

📝 l0rinc opened a pull request: "fuzz: provide more realistic values to the base58(check) decoders"
(https://github.com/bitcoin/bitcoin/pull/31917)
This is a follow-up to https://github.com/bitcoin/bitcoin/pull/30746, expanding coverage by:
* providing more valid values to the decoder (suggested by @maflcko in https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1957847712) by randomly providing a random input or a valid encoded one;
* capping max sizes to `100` instead of `1000` (suggested by @marcofleon in https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1963718683) since most actual usage has lengths of e.g. `21`, `34`, `
...

💬 l0rinc commented on pull request "test: cover base[32|58|64] with symmetric roundtrip fuzz (and padding) tests":
(https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1963935857)
Pushed https://github.com/bitcoin/bitcoin/pull/31917, thanks @maflcko & @marcofleon.

👍 pinheadmz approved a pull request: "test: fix TestShell initialization and reset()"
(https://github.com/bitcoin/bitcoin/pull/31415#pullrequestreview-2630480382)
ACK 303f8cca0568d2ca77189c4eccdfc7a6913c958d

Reviewed changes since last ACK, re-reviewed all changes, tested locally by creating a `TestShell` and sending RPC commands

<details><summary>Show Signature</summary>

```
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

ACK 303f8cca0568d2ca77189c4eccdfc7a6913c958d
-----BEGIN PGP SIGNATURE-----

iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAme3V0wACgkQ5+KYS2KJ
yTqd1xAA1DppfrHcaHcdKp/HnsicUqMslnuQ6sKFoQK76EYEsQFTyjJi2QDno8iB
g4LI7wpfse3
...

💬 marcofleon commented on pull request "fuzz: provide more realistic values to the base58(check) decoders":
(https://github.com/bitcoin/bitcoin/pull/31917#discussion_r1963946848)
The problem actually turned out to be this. The input that times out is about 1MB so passing in the whole buffer causes the timeout.

💬 laanwj commented on pull request "init: Handle dropped UPnP support more gracefully":
(https://github.com/bitcoin/bitcoin/pull/31916#discussion_r1963948947)
Good catch, this is the wrong check. For the settings.json case i did consider the "is set" and "is set to true" case differently but consider them the same here.

💬 maflcko commented on pull request "init: Handle dropped UPnP support more gracefully":
(https://github.com/bitcoin/bitcoin/pull/31916#discussion_r1963961846)
I haven't tried to compile, but the modern way of my suggestion would be:

```cpp
if (const auto arg{args.GetBoolArg("-upnp")}) {
bool overridden = args.SoftSetBoolArg("-natpmp", *arg);
```

💬 instagibbs commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1963963132)
I guess my worry is that we would somehow add a regression that results in the peer set growing indefinitely in the orphanage, which would grow memory usage substantially. Here's some suggested coverage which would hopefully uncover such an issue:

```
diff --git a/src/test/fuzz/txorphan.cpp b/src/test/fuzz/txorphan.cpp
index 9133323449..df0a02d75d 100644
--- a/src/test/fuzz/txorphan.cpp
+++ b/src/test/fuzz/txorphan.cpp
@@ -45,10 +45,15 @@ FUZZ_TARGET(txorphan, .init = initialize_orphanag
...

💬 instagibbs commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1963963925)
I guess my worry is that we would somehow add a regression that results in the peer set growing indefinitely in the orphanage, which would grow memory usage substantially. Here's some suggested coverage which would hopefully uncover such an issue:

```
diff --git a/src/test/fuzz/txorphan.cpp b/src/test/fuzz/txorphan.cpp
index 9133323449..df0a02d75d 100644
--- a/src/test/fuzz/txorphan.cpp
+++ b/src/test/fuzz/txorphan.cpp
@@ -45,10 +45,15 @@ FUZZ_TARGET(txorphan, .init = initialize_orphanag
...

💬 glozow commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1963975702)
You are right. That explains why it doesn't get slower haha

💬 0xB10C commented on pull request "ci: Switch to gcr.io mirror to avoid rate limits":
(https://github.com/bitcoin/bitcoin/pull/31906#issuecomment-2672063270)
Agree that this is the better than having to modify all CI machines and deployments. I did some light testing on my runners and didn't see any problems in fetching the images. The CI passing here indicate the same. While switching the GitHub Actions tasks to the new images probably isn't necessary due to being exempt from the rate-limiting (https://github.com/bitcoin/bitcoin/pull/31906#discussion_r1961960848), I don't think it hurts.


ACK fa8de4706a01322fd8ab8e491d297b97f001ecff

💬 achow101 commented on pull request "guix: Notarize MacOS app bundle and codesign all MacOS and Windows binaries":
(https://github.com/bitcoin/bitcoin/pull/31407#issuecomment-2672063512)
According to https://developer.apple.com/forums/thread/706379, starting a command line tool from Finder just doesn't work in general.

💬 maflcko commented on pull request "qa: fix an off-by-one in utxo snapshot fuzz target and sanity check its snapshot data":
(https://github.com/bitcoin/bitcoin/pull/31910#discussion_r1963985718)
> The fuzz mock `hash[31] & 0x80 == 0` is equivalent to the pow check against regtest min difficulty used in the unit test

Just for reference, I don't think this is true. It simply happens to pass because it is the most likely outcome, given the number of regtest blocks, but that is not guaranteed. Block hashes do exist where this is not true. For example:

```cpp

{
const auto consensus = CreateChainParams(*m_node.args, ChainType::REGTEST)->GetConsensus();
unsigned int nBits{0x
...

👍 theuni approved a pull request: "cmake: Do not modify `CMAKE_TRY_COMPILE_TARGET_TYPE` globally"
(https://github.com/bitcoin/bitcoin/pull/31662#pullrequestreview-2630560726)
utACK 2c4b229c906de6250500d3af2b44808e90b9ce0b

I didn't re-review each commit in great detail, but the diff from before is as I'd expect.