💬 frankomosh commented on issue "Enable PCP by default?":
(https://github.com/bitcoin/bitcoin/issues/31663#issuecomment-2746185212)
My home router model: Huawei EchoLife HG8145V5 GPON Terminal
and I am using Linux (Ubuntu 24.04.2 LTS)
Version tested is v29.0rc2
Test result : Failed - No PCP response, no IPv6 gateway detected

`bitcoind29 -regtest -natpmp=1 -debug=net -maxconnections=60
...
[net] portmap: gateway [IPv4]: 192.168.100.1
[net] pcp: Requesting port mapping for addr 0.0.0.0 port 18444 from gateway 192.168.100.1
[net] pcp: Internal address after connect: 192.168.100.47
[net:warning] pcp: Could not receive respons
...

🚀 ryanofsky merged a pull request: "Benchmark Chainstate::ConnectBlock duration"
(https://github.com/bitcoin/bitcoin/pull/31689)

💬 ryanofsky commented on pull request "Benchmark Chainstate::ConnectBlock duration":
(https://github.com/bitcoin/bitcoin/pull/31689#issuecomment-2746207733)
> (before merging please fix the typo in the PR description)

Note: I edited out an extra comma in the description in case that was the typo, but PR otherwise looked ready so I merged it

👍 ryanofsky approved a pull request: "rpc: Allow fullrbf fee bump in (psbt)bumpfee"
(https://github.com/bitcoin/bitcoin/pull/31953#pullrequestreview-2708594899)
Code review ACK fa16051eac9a4bc1a7e0234b65d615e655ff7cf0, but I was confused by a documentation change here, and think it would be nice to update the PR to make that change clearer, and also fix the other typo that was pointed out.

💬 ryanofsky commented on pull request "rpc: Allow fullrbf fee bump in (psbt)bumpfee":
(https://github.com/bitcoin/bitcoin/pull/31953#discussion_r2009125904)
In commit "rpc: Allow fullrbf fee bump" (fa16051eac9a4bc1a7e0234b65d615e655ff7cf0)

New comment here seems right, and old comment here seems wrong, but I'm confused about how code got in the current state. It looks like maybe the documentation here was written before #15557 and #15557 tried to preserve the behavior that's described, but the code added in #15557 to do that was dead and didn't work. Then the dead code was removed in #24562 and the documentation was not updated then either. Now t
...

💬 darosior commented on issue "Enable PCP by default?":
(https://github.com/bitcoin/bitcoin/issues/31663#issuecomment-2746249413)
Thanks everyone for the testing so far. @frankomosh can you confirm the [UPnP setting](https://manualsfile.com/product/k2c3wh8nad7.html#p44) is enabled on your router? I want to differentiate between routers with UPnP disabled by default and those that don't support PCP/NAT-PMP.

💬 grubles commented on issue "bitcoind immediately segfaults on ppc64 openbsd 7.4":
(https://github.com/bitcoin/bitcoin/issues/29517#issuecomment-2746252612)
FWIW I tried building on PPC64 Linux and `bitcoind` does **not** immediately crash. So it looks like something specific to OpenBSD powerpc64.

💬 maflcko commented on issue "bitcoind immediately segfaults on ppc64 openbsd 7.4":
(https://github.com/bitcoin/bitcoin/issues/29517#issuecomment-2746257914)
> `bitcoind`, `bitcoin-cli`, `bitcoin-util`, and `bitcoin-wallet` crash. `bitcoin-tx` does not seem to crash but I haven't tested it much.

If you want to debug this further and gdb doesn't help you with finding the place where the crash occurs you can try to place `std::cout << __FILE__ << ":" << __LINE__ << std::endl;` in the source code and then "bisect" from there until the exact line in the source code is known.

🚀 ryanofsky merged a pull request: "Doc: add a comment referencing past vulnerability next to where it was fixed"
(https://github.com/bitcoin/bitcoin/pull/30538)

💬 ryanofsky commented on pull request "Doc: add a comment referencing past vulnerability next to where it was fixed":
(https://github.com/bitcoin/bitcoin/pull/30538#issuecomment-2746267360)
Merged with 2 stale acks since there were not any real changes to the PR, checking with `git range-diff origin/master 7fea8eeeb9984ff6f3ed661f3970b1aaa68548de eb0724f0dee307d6d14e47ebd3077b7ffd50f507`. It was just rebased due to a conflict nearby.

💬 frankomosh commented on issue "Enable PCP by default?":
(https://github.com/bitcoin/bitcoin/issues/31663#issuecomment-2746268475)
> Thanks everyone for the testing so far. [@frankomosh](https://github.com/frankomosh) can you confirm the [UPnP setting](https://manualsfile.com/product/k2c3wh8nad7.html#p44) is enabled on your router? I want to differentiate between routers with UPnP disabled by default and those that don't support PCP/NAT-PMP.

I confirmed. UPnP its not enabled on my router

👍 ryanofsky approved a pull request: "Have createNewBlock() wait for tip, make rpc handle shutdown during long poll and wait methods"
(https://github.com/bitcoin/bitcoin/pull/31785#pullrequestreview-2708619308)
Code review ACK cc1001f3bf17b31512c05fb359e09483a07fb2a3. Only change since last review was rebasing after #31283

re: https://github.com/bitcoin/bitcoin/pull/31785#pullrequestreview-2702397335

> is a use for returning the last known tip?

I think the main use for returning the last known tip when the tip hasn't changed is just to allow the caller distinguish between the case where the tip has not changed from the case where the node is shutting down. If it returned null in both cases we
...

💬 ryanofsky commented on pull request "Have createNewBlock() wait for tip, make rpc handle shutdown during long poll and wait methods":
(https://github.com/bitcoin/bitcoin/pull/31785#discussion_r2009145946)
In commit "Have createNewBlock() wait for a tip" (db14ca3556ca792546bf4343feb733271333690f)

Commit message is a little confusing because it doesn't mention the timeout change. Would be clearer if it said the commit was changing two things (1) returning null on shutdown instead of last tip (2) ignoring timeout value during startup instead of returning 0 if timeout elapsed before tip was connected

👍 ryanofsky approved a pull request: "rpc: add optional blockhash to waitfornewblock, unhide wait methods in help"
(https://github.com/bitcoin/bitcoin/pull/30635#pullrequestreview-2708626067)
Code review ACK e47b20f2f310f05832c879401660860cc40a6a09 with no changes since last review other than rebase

🤔 ryanofsky reviewed a pull request: "test: add missing segwitv1 test cases to `script_standard_tests`"
(https://github.com/bitcoin/bitcoin/pull/31340#pullrequestreview-2708628256)
@theStack this has two ACKs but also some unaddressed comments. It otherwise seems ok to merge since it is just a test change and small refactoring. Do you want to update the pr or respond to the comments?

💬 yancyribbens commented on issue "BnB untested/unused condition in UTXO exclusion optimization":
(https://github.com/bitcoin/bitcoin/issues/32047#issuecomment-2746287437)
> Yeah, I have had in my backlog that I would like to refactor BnB into the style of CoinGrinder since I wrote CoinGrinder, because I think it would be easier to understand, the iteration count would make more sense (currently it counts and evaluates the backtracking steps as well, but they can never yield a new solution!), and maybe easier to test. Given that my new job is explicitly more focused on working on the wallet, I was hoping to get to that once we are in the new office in April, but i
...

👍 ryanofsky approved a pull request: "CLI cleanups"
(https://github.com/bitcoin/bitcoin/pull/31887#pullrequestreview-2708629412)
Code review ACK d423fd9ec83ae323ac29bdc1b677ed8260bd59c4, just running clang-format and updating commit messages since last review

👍 ryanofsky approved a pull request: "Execute Discover() when bind=0.0.0.0 or :: is set"
(https://github.com/bitcoin/bitcoin/pull/31492#pullrequestreview-2708633985)
Code review ACK 1561575c2d984fd94c9679de3d1de207ef6c7c06, but would be nice to have feedback from @vasild here since this is supposed to fix #31293.

The changes here all seem to make sense but the interactions between them are complicated. I think this PR would be easier to review if the changes could be broken down as much as possible. It would be nice to add new tests in a first commit to clarify current buggy behaviors. Then it seems like the GetListenPort =onion parsing fix could be a sep
...

💬 ryanofsky commented on pull request "Execute Discover() when bind=0.0.0.0 or :: is set":
(https://github.com/bitcoin/bitcoin/pull/31492#discussion_r2009158096)
Might be missing something, but it seems like this could be simplified:

```c++
const CService onion_addr{DefaultOnionServiceTarget(GetListenPort() + 1);
```

🚀 ryanofsky merged a pull request: "CLI cleanups"
(https://github.com/bitcoin/bitcoin/pull/31887)