💬 Sjors commented on pull request "interfaces: refactor: move `Mining` and `BlockTemplate` implementation to miner":
(https://github.com/bitcoin/bitcoin/pull/32378#discussion_r2089392199)
As long as getTip() is in the interface it seems consistent with the rest of this PR to move the implementation, so no need to drop c39ca9d4f7bc9ca155692ac949be2e61c0598a97?

Meanwhile you suggestion can be still be used in the 5th commit.

💬 vasild commented on pull request "Replace libevent with our own HTTP and socket-handling implementation":
(https://github.com/bitcoin/bitcoin/pull/32061#discussion_r2089410867)
Bikeshed, feel free to ignore.

It is redundant to have "http" in both the namespace and in the class name: `http_bitcoin::HTTPHeaders::Find()`. Also no need to suffix anything with `_bitcoin` - if we would do that, then we would have to have e.g. `namespace common_bitcoin`, `namespace i2p_bitcoin`, `namespace node_bitcoin` etc. So `http_bitcoin::HTTPHeaders::Find()` could be shortened to `http::Headers::Find()`.

💬 ismaelsadeeq commented on pull request "interfaces: refactor: move `Mining` and `BlockTemplate` implementation to miner":
(https://github.com/bitcoin/bitcoin/pull/32378#discussion_r2089411844)
yeah it is not, I will do if I have to retouch.

💬 ismaelsadeeq commented on pull request "interfaces: refactor: move `Mining` and `BlockTemplate` implementation to miner":
(https://github.com/bitcoin/bitcoin/pull/32378#discussion_r2089413630)
I will do if I will retouch as well.

💬 ryanofsky commented on pull request "interfaces: refactor: move `Mining` and `BlockTemplate` implementation to miner":
(https://github.com/bitcoin/bitcoin/pull/32378#discussion_r2089419101)
re: https://github.com/bitcoin/bitcoin/pull/32378#discussion_r2089392199

> As long as getTip() is in the interface it seems consistent with the rest of this PR to move the implementation, so no need to drop [c39ca9d](https://github.com/bitcoin/bitcoin/commit/c39ca9d4f7bc9ca155692ac949be2e61c0598a97)?

Yes that's fine since it's possible the function could be used other places. I just suggested dropping the commit to shrink the PR since getTip method is already pretty simple and doesn't nece
...

📝 BrandonOdiwuor opened a pull request: "RPC: removeprunedfunds should take an array of txids"
(https://github.com/bitcoin/bitcoin/pull/32501)
Fixes https://github.com/bitcoin/bitcoin/issues/29466

Update `removeprunedfunds` RPC to take an `array of strings of txids` instead of a `single txid string` to allow batch removal of transactions

✅ maflcko closed an issue: "test: WARNING: ThreadSanitizer: lock-order-inversion (potential deadlock) (pid=32090)"
(https://github.com/bitcoin/bitcoin/issues/30764)

💬 maflcko commented on issue "test: WARNING: ThreadSanitizer: lock-order-inversion (potential deadlock) (pid=32090)":
(https://github.com/bitcoin/bitcoin/issues/30764#issuecomment-2881049570)
> It happens since [fa38664](https://github.com/bitcoin/bitcoin/commit/fa386642b4dfd88f74488c288c7886494d69f4ed), which added a missing lock on cs_main.

It is fixed since e758b26b85da27ef44f3d2c924f3f08e8c1f4fdf, which I can't explain. But this was a false positive anyway.


The fixed commit is using cmake and I used the command on a fresh install of Ubuntu 24.04:

`rm -rf ./bld-cmake/ && cmake -B ./bld-cmake -DCMAKE_C_COMPILER='clang' -DCMAKE_CXX_COMPILER='clang++' -DBUILD_FUZZ_BINARY=OFF -DB
...

💬 maflcko commented on issue "test: WARNING: ThreadSanitizer: lock-order-inversion (potential deadlock) (pid=32090)":
(https://github.com/bitcoin/bitcoin/issues/30764#issuecomment-2881055089)
> I might be seeing this today as well: https://gist.github.com/pinheadmz/08758d268ec1741afc8c4f181c0e97d2
>
> My branch forks off of master at [db36a92](https://github.com/bitcoin/bitcoin/commit/db36a92c02b83f2e6477a5a55fc061319f7cc6a3)

I don't think this is related. It looks like some kind of BDB issue. However, this issue is about a different trace, see https://github.com/bitcoin/bitcoin/issues/30764#issuecomment-2321313037.

Your issue should be fixed as well, now that BDB is removed.

If
...

👍 theuni approved a pull request: "refactor: Remove UB in prevector reverse iterators"
(https://github.com/bitcoin/bitcoin/pull/32490#pullrequestreview-2841063795)
utACK faf9082a5f689e2e51a474bf654e4e9b6ca29685

I didn't test for the UB, the argument for removal is good enough for me.

```c++
prevector<42, char> foo;
std::ranges::reverse_view rv{foo};
```

^^ works

💬 achow101 commented on pull request "wallet: Ensure best block matches wallet scan state":
(https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2089478442)
Those usages of `ScanForWalletTransactions` specifically set the option to not write the last block scanned to disk. This is because they are expected to be used during normal operation when the wallet will still be handling incoming blocks and transactions, so having that record possibly be out sync is actually quite bad.

💬 achow101 commented on pull request "wallet: Ensure best block matches wallet scan state":
(https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2089478709)
Indeed, done.

💬 pinheadmz commented on pull request "Replace libevent with our own HTTP and socket-handling implementation":
(https://github.com/bitcoin/bitcoin/pull/32061#discussion_r2089484885)
Happy to change the names, I just needed to separate the old libevent server from the new one in the PR in the series of commits where each namespace has classes like `HTTPRequest`

💬 maflcko commented on issue "test: wallet_reorgsrestore.py failure under valgrind":
(https://github.com/bitcoin/bitcoin/issues/32493#issuecomment-2881115243)
Ubuntu Archives were down today, so I applied some patches before reproducing:

```diff
diff --git a/ci/test/01_base_install.sh b/ci/test/01_base_install.sh
index 1344563268..49aeeacf53 100755
--- a/ci/test/01_base_install.sh
+++ b/ci/test/01_base_install.sh
@@ -21,6 +21,9 @@ if [ -n "$DPKG_ADD_ARCH" ]; then
dpkg --add-architecture "$DPKG_ADD_ARCH"
fi

+apt update && apt install ca-certificates -y
+echo -e "# Packages and Updates from the Hetzner Ubuntu Mirror\ndeb https://mirror.hetzner.co
...

📝 davidgumberg opened a pull request: "wallet: Drop unused fFromMe from CWalletTx"
(https://github.com/bitcoin/bitcoin/pull/32502)
This has been unused since commit fe52346, this is a re-opening of #9351 by @ryanofsky.

💬 mzumsande commented on pull request "wallet: Ensure best block matches wallet scan state":
(https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2089511246)
Also just mentioning that `rescanblockchain` can be used with ranges (`start_height` / `stop_height`). If `stop_height` is set to some past block, setting the best block back to that old block would be incorrect and lead to wrong balances etc.

💬 achow101 commented on pull request "rpc: Undeprecate rpcuser/rpcpassword, store all credentials hashed in memory":
(https://github.com/bitcoin/bitcoin/pull/32423#discussion_r2089511624)
In 98ff38a6f1a8a1e214bd3905a2dcac31ae6c2f52 "rpc: Perform HTTP user:pass split once in `RPCAuthorized`"

Could use `util::Split` instead of reimplementing split

💬 maflcko commented on pull request "wallet: Drop unused fFromMe from CWalletTx":
(https://github.com/bitcoin/bitcoin/pull/32502#issuecomment-2881173072)
lgtm ACK 5bf91ba8800d23402536d758f02198eac0fd7d61

💬 maflcko commented on pull request "wallet: Drop unused fFromMe from CWalletTx":
(https://github.com/bitcoin/bitcoin/pull/32502#issuecomment-2881175789)
(please no `@` in the pull description, otherwise people will get pinged for every cherry-pick of the merge commit, which includes this)

💬 davidgumberg commented on pull request "wallet: Drop unused fFromMe from CWalletTx":
(https://github.com/bitcoin/bitcoin/pull/32502#issuecomment-2881180201)
> (please no `@` in the pull description, otherwise people will get pinged for every cherry-pick of the merge commit, which includes this)

Fixed, thanks.