💬 furszy commented on issue "wallet: setting changes are subject to race conditions":
(https://github.com/bitcoin/bitcoin/issues/30620#issuecomment-2293710537)
@ismaelsadeeq, the race is one level above.
`AddWalletSetting` obtains a copy of the 'wallet' settings json array, modifies it without any lock, and then calls `updateRwSetting`, which entirely replaces the existing value. If two threads access `AddWalletSetting` simultaneously, they both obtain the same 'wallet' settings array, append their new wallet, and store it. Only the data from the thread that executes the write operation last survives.
The same issue occurs on `RemoveWalletSetting`. T
...

✅ glozow closed an issue: "Follow-up ideas for XOR blocksdir *.dat files"
(https://github.com/bitcoin/bitcoin/issues/30599)

🚀 glozow merged a pull request: "test: add functional test for XORed block/undo files (`-blocksxor` option)"
(https://github.com/bitcoin/bitcoin/pull/30657)

💬 sipa commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1719998377)
Fixed.

👍 ryanofsky approved a pull request: "Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange and drop CRPCSignals"
(https://github.com/bitcoin/bitcoin/pull/30409#pullrequestreview-2242860642)
Code review ACK 9ba60b3a2c22dcd1c9e7b4880f0adb678e4e8d63. Just fixed a few bugs to make waiting more reliable since last review and changed test logging. I like the new change to check for overflows generally in waitTipChanged, instead of just checking specifically for the max timeout value. That was better than the suggestion.

💬 ryanofsky commented on pull request "Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange and drop CRPCSignals":
(https://github.com/bitcoin/bitcoin/pull/30409#discussion_r1719939574)
In commit "Add waitTipChanged to Mining interface" (d1c197fbbad86e3f966d9bf9abe4dd0aa76bc83d)

Think it would be simpler to say "returns hash and height of the current chain tip after this call."

It seems potentially misleading to refer to a "previous" tip here because that sounds like this could sometimes return an old block hash. And it's a little confusing to refer to a "new "tip in the case where this function just returns immediately and there no interruption or wait or timeout.

💬 ryanofsky commented on pull request "Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange and drop CRPCSignals":
(https://github.com/bitcoin/bitcoin/pull/30409#discussion_r1719929110)
In commit "Add waitTipChanged to Mining interface" (d1c197fbbad86e3f966d9bf9abe4dd0aa76bc83d)

Not sure "in case of race condition" actually explains this. Would suggest changing this to:

- `current_tip` - block hash compared against the current chain tip. Function waits for the chain tip to change if this matches, otherwise it returns right away.

💬 ryanofsky commented on pull request "Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange and drop CRPCSignals":
(https://github.com/bitcoin/bitcoin/pull/30409#discussion_r1719963458)
In commit "Add waitTipChanged to Mining interface" (d1c197fbbad86e3f966d9bf9abe4dd0aa76bc83d)

I think this comment is only warning about a small part of a bigger danger. It's not just unsafe to obtain the height here but unsafe to acquire cs_main anywhere in this code block, even after the while loop.

Would suggest dropping this comment and instead adding a simpler comment above LOCK(::cs_main) that says
"Must release g_best_block_mutex before locking cs_main, to avoid deadlocks."

A c
...

💬 ryanofsky commented on pull request "Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange and drop CRPCSignals":
(https://github.com/bitcoin/bitcoin/pull/30409#discussion_r1719972779)
In commit "Add waitTipChanged to Mining interface" (d1c197fbbad86e3f966d9bf9abe4dd0aa76bc83d)

Not important but I think it would be clearer to condense these 3 lines to just:

```c++
g_best_block_cv.wait_until(lock, std::min(deadline, now + tick));
```

I don't think it helps to introduce 2 extra variables and subtract `now` then add `now` back.

💬 achow101 commented on pull request "seeds: Pull additional nodes from my seeder and update fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/30008#discussion_r1720001422)
Updated `MIN_BLOCKS`.

The seeders don't necessarily have their own node.

💬 achow101 commented on pull request "seeds: Pull additional nodes from my seeder and update fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/30008#discussion_r1720001491)
Fixed

💬 achow101 commented on pull request "seeds: Pull additional nodes from my seeder and update fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/30008#discussion_r1720001748)
I've commented it out and added a todo.

💬 achow101 commented on pull request "seeds: Pull additional nodes from my seeder and update fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/30008#discussion_r1720002689)
Yes, if it's old, we should use a more up to date one. Can that repo somehow have a permalink to the most recent asmap? Otherwise we would run into the same problem with outdated asmaps.

💬 fjahr commented on pull request "kernel: pre-28.x chainparams and headerssync update":
(https://github.com/bitcoin/bitcoin/pull/30658#issuecomment-2293729791)
re-ACK 221809b81cfcecb04050915eebacffda2599da42

Only changes since last review were bumping the Testnet3 size numbers and making `headerssync-params.py` executable.

https://github.com/bitcoin/bitcoin/compare/cd913de6488d25057e3bf7dcad1508e9d689f87f..221809b81cfcecb04050915eebacffda2599da42

👍 ryanofsky approved a pull request: "ci: skip Github CI on branch pushes for forks"
(https://github.com/bitcoin/bitcoin/pull/30487#pullrequestreview-2243000916)
Code review ACK 3067cd55f0fc79a7fa6342ca0600472cabcf2690.

Thanks for the explanation, that all sounds good to me.

The new description is definitely better than the original but it is not actually saying what behavior will be after this PR. Would be helpful to say something like "After this PR, pushes made to git branches inside fork repositories will no longer trigger CI runs, unless the git branches are associated with PRs in the fork repository, or the main repository." (assuming that is
...

🤔 maflcko reviewed a pull request: "refactor: Replace ParseHex with consteval HexLiteral"
(https://github.com/bitcoin/bitcoin/pull/30377#pullrequestreview-2242829972)
review ACK 44bb5a12c4389dc18e181387356901787844e89f

(I'll do the rest later)

💬 maflcko commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1719924332)
nit in https://github.com/bitcoin/bitcoin/commit/1dddb4fb0ff88162e58e50e56d5e889bb5727a81: Wrong rename?

💬 maflcko commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1719915623)
nit in https://github.com/bitcoin/bitcoin/commit/1dddb4fb0ff88162e58e50e56d5e889bb5727a81: Use `std::span` to avoid having to touch this again in the future for that reason?

💬 maflcko commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1720015406)
nit in 82c9080f235f6ffbe983b1360dc5741ce95e9a7d: Follow-up nit: I think it was me who suggested to avoid array/span serialization in script, but I think it could be considered when the vector serialization is mirrored. I understand that this different from the `serialize.h` serialization, but the two are separate anyway (and script inherits the whole prevector functions to insert raw bytes at any point anyway).

In any case, this doesn't remove the need for `Vec` and can be done in a follow-up
...

💬 maflcko commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1719910914)
nit in 1dddb4fb0ff88162e58e50e56d5e889bb5727a81: Any reason to remove the comment? Seems important to keep, otherwise a dev may be surprised when the function crashes when it isn't exactly 32 bytes.