💬 MarcoFalke commented on pull request "rpc: Fix invalid bech32 address handling":
(https://github.com/bitcoin/bitcoin/pull/27727#issuecomment-1563047960)
i guess 22.x is EOL, so I removed that

💬 achow101 commented on pull request "wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction":
(https://github.com/bitcoin/bitcoin/pull/25273#issuecomment-1563059936)
Rebased

🤔 TheCharlatan requested changes to a pull request: "Return EXIT_FAILURE on post-init fatal errors"
(https://github.com/bitcoin/bitcoin/pull/27708#pullrequestreview-1443959388)
Was wondering if this was attempted before and skimmed through the pull request history, but I did not find prior attempts.

💬 TheCharlatan commented on pull request "Return EXIT_FAILURE on post-init fatal errors":
(https://github.com/bitcoin/bitcoin/pull/27708#discussion_r1205560303)
Could this just be a local static function in shutdown.cpp and not exposed in the header?

💬 TheCharlatan commented on pull request "Return EXIT_FAILURE on post-init fatal errors":
(https://github.com/bitcoin/bitcoin/pull/27708#discussion_r1205561022)
Does it make sense to add `[[nodiscard]]` here?

💬 TheCharlatan commented on pull request "Return EXIT_FAILURE on post-init fatal errors":
(https://github.com/bitcoin/bitcoin/pull/27708#discussion_r1205612087)
This comment seems a bit inaccurate, e.g. we may have `stop_at_height` set, but shutdown is still triggered internally. There is also one error case in https://github.com/bitcoin/bitcoin/blob/master/src/node/blockstorage.cpp#L931 that triggers a normal shutdown.

💬 pinheadmz commented on pull request "wallet: improve IBD sync time by skipping block scanning prior birth time":
(https://github.com/bitcoin/bitcoin/pull/27469#issuecomment-1563088762)
> This PR is not modifying the rescan process. Only IBD.

sanity check: because we already skip old blocks during rescan?

https://github.com/bitcoin/bitcoin/pull/26679

📝 hebasto opened a pull request: "Fix `#include`s in `src/wallet`"
(https://github.com/bitcoin/bitcoin/pull/27759)
This PR is a minimum required changes to fix https://github.com/bitcoin/bitcoin/pull/27571#discussion_r1195497290.

💬 mzumsande commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#issuecomment-1563115012)
> https://cirrus-ci.com/task/4751246913961984?logs=ci#L2724:
>
> ```shell
> test/denialofservice_tests.cpp(134): Entering test case "stale_tip_peer_management"
> test/denialofservice_tests.cpp(212): error: in "denialofservice_tests/stale_tip_peer_management": check vNodes[i]->fDisconnect == false has failed
> test/denialofservice_tests.cpp(214): error: in "denialofservice_tests/stale_tip_peer_management": check vNodes[max_outbound_full_relay - 2]->fDisconnect == true has failed
> test/den
...

💬 MarcoFalke commented on pull request "log: don't log total disk read time in ConnectTip bench":
(https://github.com/bitcoin/bitcoin/pull/27673#issuecomment-1563172416)
lgtm ACK bc862fad294fdb3e4232734497d0693a0da4d63a 🐓

<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: lgtm ACK bc862fad294fdb3e
...

💬 D33r-Gee commented on issue "25.0 RC Testing Guide Feedback":
(https://github.com/bitcoin/bitcoin/issues/27736#issuecomment-1563183052)
OS: Ubuntu 20.04

Hello found an issue running the bonus test ["Test Command specified by shutdownnotify..."](https://github.com/bitcoin-core/bitcoin-devwiki/wiki/25.0-Release-Candidate-Testing-Guide#test-command-specified-by-shutdownnotify-are-executed-synchronously-before-shutdown)

The following command didn’t work:
bitcoind-test -daemon -shutdownnotify="bcli touch hello.txt"

However after looking at the [#23395](https://github.com/bitcoin/bitcoin/pull/23395) PR, removing the bcli wor
...

💬 joostjager commented on pull request "Allow accepting non-standard transactions on mainnet":
(https://github.com/bitcoin/bitcoin/pull/27578#issuecomment-1563206281)
I understand and appreciate the concerns about DoS protection and the importance of maintaining standardness rules for network-wide transaction relay. But isn't there room for flexibility when it comes to how Bitcoin nodes handle non-standard transactions submitted locally via the RPC methods sendrawtransaction and submitpackage?

These RPC calls are made by the node operator themselves or applications they trust, thus the risk of DoS attacks is significantly mitigated. The operator can assume
...

💬 MarcoFalke commented on issue "Spurious (?) valgrind failure for p2p_compactblocks.py":
(https://github.com/bitcoin/bitcoin/issues/27741#issuecomment-1563230842)
Looks like valgrind 3.21 still has this issue. I tried on Tumbleweed OpenSuse:

```
zypper in -y valgrind libevent-devel boost-devel find bison gcc-c++ libtool make autoconf automake python3 lbzip2 patch xz curl wget htop git vim ccache && git clone https://github.com/bitcoin/bitcoin.git --depth=1 ./bitcoin-core && cd bitcoin-core && ./autogen.sh && ./configure --disable-tests --disable-bench && make -j $(nproc) && ./test/functional/p2p_compactblocks.py --valgrind
```


...

💬 achow101 commented on pull request "wallet: when a block is disconnected, update transactions that are no longer conflicted":
(https://github.com/bitcoin/bitcoin/pull/27145#issuecomment-1563232666)
ACK 89df7987c2f1eea42454c2b0efc31a924fbfd3a8

💬 darricksee commented on issue "Frequent "Timeout downloading block" with 24.1":
(https://github.com/bitcoin/bitcoin/issues/27705#issuecomment-1563336061)
I'd love to see the addition of `addr` too!

💬 achow101 commented on pull request "wallet, bench: Move commonly used functions to their own file and fix a bug":
(https://github.com/bitcoin/bitcoin/pull/27666#issuecomment-1563342926)
> Maybe instead of creating a new file, we could move the bench functions to `wallet/test/util.h` and `wallet/test/util.cpp` so they are used equally for tests and benchmarks?

Good point, made that change.

It required a few changes to the `CreateWallet` test since that wasn't expecting `postInitProcess` to be run, but should be straightforward to review.

💬 achow101 commented on pull request "wallet, bench: Move commonly used functions to their own file and fix a bug":
(https://github.com/bitcoin/bitcoin/pull/27666#discussion_r1205885446)
Dropped `wallet_common.h`, but still ended up doing this in `wallet_loading` and `wallet_balance`

💬 furszy commented on pull request "wallet: improve IBD sync time by skipping block scanning prior birth time":
(https://github.com/bitcoin/bitcoin/pull/27469#issuecomment-1563383911)
> sanity check: because we already skip old blocks during rescan?
>
> https://github.com/bitcoin/bitcoin/pull/26679

Yes. Different entry points.

#26679 addressed the scanning of the existing chain during load time.
And here, the emphasis shifts to the synchronization process when the
wallet is active.
When the node activate the best chain on each processed block and
signal the "block connected" event.

📝 mzumsande opened a pull request: "p2p: Log addresses of stalling peers"
(https://github.com/bitcoin/bitcoin/pull/27761)
This was suggested in #27705 by ArmchairCryptologist.
It allows node operators that have the `-logips` option enabled to better identify potentially misbehaving peers and maybe ban them, which is especially helpful in case of inbound peers for which (dis)connections aren't logged per default, so it's impossible to use the debug log to connect their nodeId to an address unless the very noisy NET debugging is enabled.
In case of outbound peers for which the address is potentially logged when est
...

💬 brunoerg commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#issuecomment-1563416721)
> Ah, I think the problem with the test is that AddRandomOutboundPeer creates random IPv4 addresses, and sometimes these aren't routable (e.g. protected ranges), so that they are not assigned to NET_IPV4 but NET_UNROUTABLE, which then makes the test fail. This happens intermittently. @amitiuttarwar I haven't looked into solutions yet, but may just try another IPv4 address if this is the case?

Not sure, I ran the test several times (debugging) and could get the error but didn't get a `NET_UNRO
...