👍 ryanofsky approved a pull request: "assumeutxo: Add dumptxoutset height param, remove shell scripts"
(https://github.com/bitcoin/bitcoin/pull/29553#pullrequestreview-2254925960)
Code review ACK b5292334e5e8792644123b627896359eb1da8d25, just updated code comments since last review.

---

re: https://github.com/bitcoin/bitcoin/pull/29553#pullrequestreview-2253922313

> Does CCoinsViewCursor (and LevelDB) somehow guarantee that if coins are deleted from the chainstate database, as new blocks are processed, we can still access them? (and that we won't see newly added coins).

CCoinsViewCursor is a dumb cursor, so leveldb would be doing all the work, but my understan
...

💬 ryanofsky commented on pull request "assumeutxo: Add dumptxoutset height param, remove shell scripts":
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1727333039)
re: https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1725892812

Interesting results and I agree it makes sense to leave this alone for now. I thought it adding an option might be reasonable because doing this might increase resource usage even if it is faster. And maybe it is better if node is unusable for longer as Sjors says. I don't have a clear sense of what is best here.

💬 maflcko commented on pull request "Wallet, Bugfix: Lock Wallet Context mutex Before Adding/Removing Settings":
(https://github.com/bitcoin/bitcoin/pull/30697#discussion_r1727394440)
I haven't looked at the CI failure in detail, but IIRC, `BOOST_CHECK` macros are not allowed to be used in threads, because they are not thread-safe.

Does the race go away if you use `Assert` instead? Alternatively, you could try to collect the results and `BOOST_CHECK` them after the threads have joined

👍 ryanofsky approved a pull request: "fix: handle invalid `-rpcbind` port earlier"
(https://github.com/bitcoin/bitcoin/pull/30679#pullrequestreview-2255028620)
Code review ACK 51ba0e61273bec02833329ee6ac8fa219679cefa. Just split up the change and moved some code so this should be a little easier to review now.

I had another idea and left a suggestion but it is not important so please feel free to ignore.

💬 ryanofsky commented on pull request "fix: handle invalid `-rpcbind` port earlier":
(https://github.com/bitcoin/bitcoin/pull/30679#discussion_r1727402857)
Code review ACK 51ba0e61273bec02833329ee6ac8fa219679cefa

(Not important, but another idea to make this commit smaller and clearer: I think it would be good to move the change in `src/httpserver.cpp` to a separate followup commit, because while it's a good change that improves code quality and error reporting, it is not really a part of the bugfix, and is more tangentially related to it.)

💬 ismaelsadeeq commented on pull request "Wallet, Bugfix: Lock Wallet Context mutex Before Adding/Removing Settings":
(https://github.com/bitcoin/bitcoin/pull/30697#discussion_r1727405739)
I suppose I should update to check for the boolean returned value of true not a string message.

I will update to just `Assert` the returned value is true.

I will test your suggestion, thanks

🤔 stickies-v reviewed a pull request: "Wallet, Bugfix: Lock Wallet Context mutex Before Adding/Removing Settings"
(https://github.com/bitcoin/bitcoin/pull/30697#pullrequestreview-2254912898)
Approach ACK

💬 stickies-v commented on pull request "Wallet, Bugfix: Lock Wallet Context mutex Before Adding/Removing Settings":
(https://github.com/bitcoin/bitcoin/pull/30697#discussion_r1727323124)
For stability, would ensure we start without any wallets (e.g. in case any defaults get added, will make it more visible why this test fails. Alternatively, could capture the count of wallets at the beginning of the test and use that to compare with later on, to make the test resilient to unrelated changes, but I think that might be a bit overkill.

```suggestion
std::vector<std::thread> threads;
// Since we're counting the number of wallets, ensure we start without any.
BOOST_R
...

💬 stickies-v commented on pull request "Wallet, Bugfix: Lock Wallet Context mutex Before Adding/Removing Settings":
(https://github.com/bitcoin/bitcoin/pull/30697#discussion_r1727341640)
Would be good to guard against lock contention (+ for `RemoveWalletSetting`):

<details>
<summary>git diff on 135eb1d5b1</summary>

```diff
diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h
index ad2e2817c4..fd61b39c47 100644
--- a/src/wallet/wallet.h
+++ b/src/wallet/wallet.h
@@ -19,6 +19,7 @@
#include <script/script.h>
#include <support/allocators/secure.h>
#include <sync.h>
+#include <threadsafety.h>
#include <tinyformat.h>
#include <uint256.h>
#include <util/fs.h>
...

💬 stickies-v commented on pull request "Wallet, Bugfix: Lock Wallet Context mutex Before Adding/Removing Settings":
(https://github.com/bitcoin/bitcoin/pull/30697#discussion_r1727326765)
nit: brief docstrings to at-a-glance help see how the test is structured:

<details>
<summary>git diff on 135eb1d5b1</summary>

```diff
diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp
index ec9792f7a9..d434c1c207 100644
--- a/src/wallet/test/wallet_tests.cpp
+++ b/src/wallet/test/wallet_tests.cpp
@@ -337,6 +337,7 @@ BOOST_FIXTURE_TEST_CASE(write_wallet_settings_concurrently, TestingSetup)
context.chain = m_node.chain.get();
const int NUM_WAL
...

💬 stickies-v commented on pull request "Wallet, Bugfix: Lock Wallet Context mutex Before Adding/Removing Settings":
(https://github.com/bitcoin/bitcoin/pull/30697#discussion_r1727401390)
```suggestion
std::vector<std::thread> threads;
threads.reserve(NUM_WALLETS);
```

💬 stickies-v commented on pull request "Wallet, Bugfix: Lock Wallet Context mutex Before Adding/Removing Settings":
(https://github.com/bitcoin/bitcoin/pull/30697#discussion_r1727410617)
I think TSAN is [failing](https://github.com/bitcoin/bitcoin/pull/30697/checks?check_run_id=29119417676) because of these `BOOST_CHECK_MESSAGE` statements. Since you're checking the count a bit further down already anyway, I think this is fine to remove. Exactly which thread failed doesn't really add information anyway, I think?

<details>
<summary>git diff on 135eb1d5b1</summary>

```diff
diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp
index ec9792f7a9..6
...

💬 stickies-v commented on pull request "Wallet, Bugfix: Lock Wallet Context mutex Before Adding/Removing Settings":
(https://github.com/bitcoin/bitcoin/pull/30697#discussion_r1727369065)
style nit: should either use braces or multi-line, or move to one-line approach

```suggestion
for (auto& t : threads) t.join();
```

💬 stickies-v commented on pull request "Wallet, Bugfix: Lock Wallet Context mutex Before Adding/Removing Settings":
(https://github.com/bitcoin/bitcoin/pull/30697#discussion_r1727414565)
As per https://github.com/bitcoin/bitcoin/pull/30697#discussion_r1727410617, I'm not sure capturing the offending thread adds value, since it won't be deterministic anyway? And we're already testing that all wallets were added/removed successfully? Also no objection to adding an `Assert`, though.

💬 fjahr commented on pull request "test: assumeutxo: add missing tests in wallet_assumeutxo.py":
(https://github.com/bitcoin/bitcoin/pull/30455#issuecomment-2305142556)
CI seems to have an issue with the import descriptor test

```
test 2024-08-22T15:25:46.161000Z TestFramework (ERROR): Unexpected exception caught during testing
Traceback (most recent call last):
File "/ci_container_base/ci/scratch/build/bitcoin-i686-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 132, in main
self.run_test()

...

🚀 achow101 merged a pull request: "Have miner account for timewarp mitigation, activate on regtest, lower nPowTargetTimespan to 144 and add test"
(https://github.com/bitcoin/bitcoin/pull/30681)

💬 ismaelsadeeq commented on pull request "Wallet, Bugfix: Lock Wallet Context mutex Before Adding/Removing Settings":
(https://github.com/bitcoin/bitcoin/pull/30697#issuecomment-2305161013)
Thanks @stickies-v for review, I've accepted all your suggestion and updated the branch.
I will force push after verifying TSan C.I passes locally

🚀 glozow merged a pull request: "kernel: pre-28.x chainparams and headerssync update"
(https://github.com/bitcoin/bitcoin/pull/30658)

💬 tdb3 commented on pull request "fix: handle invalid `-rpcbind` port earlier":
(https://github.com/bitcoin/bitcoin/pull/30679#discussion_r1727455367)
Thanks. Yes, that's correct (movement of CheckHostPortOptions() handles the bug before HTTPBindAddresses()). It's cleaner to break this out into its own commit. Updated.

🤔 ismaelsadeeq reviewed a pull request: "test: replace deprecated secp256k1 context flags usage"
(https://github.com/bitcoin/bitcoin/pull/30687#pullrequestreview-2255117049)
utACK 60055f1abc4b4ad5f66a2fcf2e61c65efc777036