💬 ajtowns commented on pull request "rpc: Add importmempool RPC":
(https://github.com/bitcoin/bitcoin/pull/27460#discussion_r1166327231)
Change "the file" to "a mempool.dat file" ?

Maybe reference the "savemempool" rpc as a way of updating mempool.dat with the current mempool?

💬 ajtowns commented on pull request "rpc: Add importmempool RPC":
(https://github.com/bitcoin/bitcoin/pull/27460#discussion_r1166274209)
Why would either of these "corrupt" the mempool state? They definitely might be undesirable ("unbroadcast" txs might reveal which node just loaded the mempool state, and "fee delta" may allow an attacker to pin txs to your mempool that will never be mined, or to get you to mine unprofitable txs), but the problems should be localised and not result in "corruption", no?

💬 ishaanam commented on pull request "wallet: when a block is disconnected, update transactions that are no longer conflicted":
(https://github.com/bitcoin/bitcoin/pull/27145#discussion_r1166333715)
> The lambda function's wtx shadows the function's wtx.

I have renamed the lambda function's `wtx`.
> Could just provide disconnect_height.

By this do you mean providing the `disconnect_height` of the block to `RecursiveUpdateTxState` or providing the wallet tx's `conflicting_block_height` to `try_updating_state` instead of the whole wallet tx by reference?

💬 ishaanam 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-1507990460)
Thanks for the review @achow101 and @furszy!
> Haven't finished checking the latest changes but, if you like it, https://github.com/furszy/bitcoin-core/commit/8bb14c1eaf09b0bca9efa78e1a7a7a00c61c0a29 is all yours. During the review, the functional test wasn't clear enough for me so I spent a bit of time making it more friendly.

Thanks, I'll use these improvements here once I review them.

💬 MarcoFalke commented on pull request "rpc: Add importmempool RPC":
(https://github.com/bitcoin/bitcoin/pull/27460#discussion_r1166365182)
Yeah, happy to change the wording, if there is a suggestion I can copy-paste?

💬 MarcoFalke commented on pull request "rpc: Add importmempool RPC":
(https://github.com/bitcoin/bitcoin/pull/27460#discussion_r1166368024)
Maybe `"Warning: Importing untrusted metadata may lead to unexpected issues and undesirable bugs. Do not set this bool unless you understand exactly what it does."`?

💬 MarcoFalke commented on pull request "rpc: Add importmempool RPC":
(https://github.com/bitcoin/bitcoin/pull/27460#discussion_r1166394731)
I guess it should also clarify that this is not a replacement for `-persistmempool`

💬 ajtowns commented on pull request "rpc: Add importmempool RPC":
(https://github.com/bitcoin/bitcoin/pull/27460#discussion_r1166428615)
"undesirable behaviour" instead of bugs -- this is a case of you telling it to do the wrong thing, so not really a bug? Otherwise sgtm.

💬 ajtowns commented on pull request "rpc: Add importmempool RPC":
(https://github.com/bitcoin/bitcoin/pull/27460#discussion_r1166430613)
(I more just meant "what the heck type of file do you want me to give it???")

💬 ajtowns commented on pull request "RPC: Accept options as named-only parameters":
(https://github.com/bitcoin/bitcoin/pull/26485#issuecomment-1508080736)
Approach ACK

(just for the record)

💬 MarcoFalke commented on issue "index: ThreadSanitizer: data race on vptr ":
(https://github.com/bitcoin/bitcoin/issues/27355#issuecomment-1508155742)
Maybe you can share your CPU model/hardware, so that someone can try on that, but other than that I am not sure what to do here

💬 fanquake commented on issue "index: ThreadSanitizer: data race on vptr ":
(https://github.com/bitcoin/bitcoin/issues/27355#issuecomment-1508165087)
> Maybe you can share your CPU model/hardware,

It's the same as yours above:
```bash
Architecture: aarch64
CPU op-mode(s): 32-bit, 64-bit
Byte Order: Little Endian
CPU(s): 4
On-line CPU(s) list: 0-3
Vendor ID: ARM
Model name: Neoverse-N1
Model: 1
Thread(s) per core: 1
Core(s) per socket: 4
Socket(s): 1
Stepping: r3p1
BogoMIPS: 243.75
Fl
...

🤔 glozow reviewed a pull request: "verify-commits: error and exit cleanly when git is too old."
(https://github.com/bitcoin/bitcoin/pull/27461#pullrequestreview-1385108023)
concept ACK

💬 fanquake commented on pull request "ci: Use clang-15 and IWYU v0.19 in "tidy" task":
(https://github.com/bitcoin/bitcoin/pull/26766#issuecomment-1508267451)
Removing up for grabs here, as we are now using clang-16 and IWYU 0.20.

📝 fanquake opened a pull request: "depends: fix compiling bdb with clang-16"
(https://github.com/bitcoin/bitcoin/pull/27462)
We've been working around this, i.e in https://github.com/bitcoin/bitcoin/issues/27355, and it makes sense to fix the build in any case.

See https://releases.llvm.org/16.0.0/tools/clang/docs/ReleaseNotes.html:
> The -Wimplicit-function-declaration and -Wimplicit-int warnings now default to an error in C99, C11, and C17. As of C2x, support for implicit function declarations and implicit int has been removed, and the warning options will have no effect. Specifying -Wimplicit-int in C89 mode wi
...

🤔 josibake reviewed a pull request: "ci: Use Cirrus CI dockerfile env"
(https://github.com/bitcoin/bitcoin/pull/27340#pullrequestreview-1385126007)
code review ACK https://github.com/bitcoin/bitcoin/pull/27340/commits/fa4a46de0b3c1a5895e95dba7e95278932fbfc2c

read through the linked documentation from cirrus and this definitely makes sense. I tried to run the ci locally using `cirrus run` but ran into some issues (unrelated to this PR). If you have any tips on how to test this, happy to give it a go, but the code looks good otherwise.

💬 josibake commented on pull request "ci: Use Cirrus CI dockerfile env":
(https://github.com/bitcoin/bitcoin/pull/27340#discussion_r1166622547)
note to self/reviewers: this file was introduced in https://github.com/bitcoin/bitcoin/pull/26976

💬 josibake commented on pull request "ci: Use Cirrus CI dockerfile env":
(https://github.com/bitcoin/bitcoin/pull/27340#discussion_r1166624499)
I grep for "TODO"s a lot when looking for stuff to work on, might be nice to add a todo comment here to make it easier for others to find later on

💬 josibake commented on pull request "ci: Use Cirrus CI dockerfile env":
(https://github.com/bitcoin/bitcoin/pull/27340#discussion_r1166633338)
this is unrelated to switching to the docker file, right? would prefer this change in it's own PR because I have a few questions about cost (before / after, etc), but not a super big deal

💬 fanquake commented on issue "index: ThreadSanitizer: data race on vptr ":
(https://github.com/bitcoin/bitcoin/issues/27355#issuecomment-1508298079)
> Hmm, then my guess that it could be the timeout was probably wrong. Could you run the test in isolation with -- -printtoconsole and post the log?