💬 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

💬 stickies-v commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#issuecomment-2305181520)
> Sorry for not understanding that you had it in the works with my prior suggestion.

No problem at all, I decided to give this PR a bit of a rest to focus on reviewing and aligning with related PRs, and think about the suggestions made here, hence the delay in updating this. Will quickly address feedback again from here on.

> The only part that still bugs me a bit is the naming of `FromUserHex` -> the "user" is on a different abstraction level, I'd prefer mentioning the behavior change com
...

💬 ryanofsky commented on pull request "Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange and drop CRPCSignals":
(https://github.com/bitcoin/bitcoin/pull/30409#discussion_r1727474250)
re: https://github.com/bitcoin/bitcoin/pull/30409#discussion_r1727105521

This is a good catch, but I"m not sure there is a observable change of behavior in `ActivateBestChain` because cs_main is held the whole time `blockTip` / `uiInterface.NotifyBlockTip` / `RPCNotifyBlockChange` is called and while the old `latestblock` global (now replaced by `g_best_block`) was updated:

https://github.com/bitcoin/bitcoin/blob/5ce2285b8739e12eebd1d53358038ec2277c662b/src/validation.cpp#L3552

So I don
...

📝 instagibbs opened a pull request: "test: Add time-timewarp-attack boundary cases"
(https://github.com/bitcoin/bitcoin/pull/30698)
Basic addition to test case added in https://github.com/bitcoin/bitcoin/pull/30681

👋 instagibbs's pull request is ready for review: "test: Add time-timewarp-attack boundary cases"
(https://github.com/bitcoin/bitcoin/pull/30698)

💬 instagibbs commented on 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#issuecomment-2305206232)
I tested this patchset with 2016 target timespan and tests passed as expected. I also added a boundary test case: https://github.com/bitcoin/bitcoin/pull/30698

💬 jonatack commented on pull request "[WIP] seeds: Add additional seed source and bump uptime requirements for Onion and I2P nodes":
(https://github.com/bitcoin/bitcoin/pull/30695#discussion_r1727515494)
I could be misreading, but it looks like all the tor and i2p peers have been removed without being added elsewhere.

💬 jonatack commented on pull request "[WIP] seeds: Add additional seed source and bump uptime requirements for Onion and I2P nodes":
(https://github.com/bitcoin/bitcoin/pull/30695#issuecomment-2305249422)
> As a result, its data contains additional eligible Onion (and other darknet nodes), as is shown in the histogram below.

Some very good tor and i2p peers seems absent from the update. I addnode them, but hm.

🤔 tdb3 reviewed a pull request: "kernel: pre-28.x chainparams and headerssync update"
(https://github.com/bitcoin/bitcoin/pull/30658#pullrequestreview-2255241049)
post merge ACK 221809b81cfcecb04050915eebacffda2599da42

Checked chainparams (mainnet and signet) manually on each of my nodes. Matched.

```
$ bitcoin-cli getblockhash 856760
$ bitcoin-cli getchaintxstats 4096 <block hash from getblockhash>
$ bitcoin-cli getblockheader <block hash from getblockhash>
$ bitcoin-cli -signet getblockhash 208800
$ bitcoin-cli -signet getchaintxstats 4096 <block hash from getblockhash>
$ bitcoin-cli -signet getblockheader <block hash from getblockhash>
``
...

📝 l0rinc opened a pull request: "test: Improve clarity of subsidy limit test"
(https://github.com/bitcoin/bitcoin/pull/30699)
The test has been divided into two distinct parts to enhance transparency and build confidence, especially for layman reviewers, in verifying Bitcoin's monetary policy finality.

First, we iterate through every single block until the subsidy reaches zero, removing confusing jumps, hard-coded block limits, and intermediary assertions.

Then, we ensure the subsidy remains zero by checking up to block 10,000,000 (skipping every 1000 blocks for efficiency).

💬 ryanofsky commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#issuecomment-2305343773)
Code review ACK 81554aac80bf2270db977c110c37acc7e8034194. Thanks for all the changes. This seems both simpler and more backward compatible now.

---

re: https://github.com/bitcoin/bitcoin/pull/30569#issue-2443189383

In PR description:

>* test: the optional `RANDOM_CTX_SEED` env var is now required to consist only of up to 64 hex digits (optionally prefixed with "0x"), otherwise the program will abort

I think it would be clearer and more consistent with the rest of the description i
...