💬 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_r1166323704)
I think that this is a good idea, but would out of the scope of this PR and would warrant more discussion, since then it would be notifying for tx state updates that it previously did not.
(https://github.com/bitcoin/bitcoin/pull/27145#discussion_r1166323704)
I think that this is a good idea, but would out of the scope of this PR and would warrant more discussion, since then it would be notifying for tx state updates that it previously did not.
💬 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_r1166323841)
Done
(https://github.com/bitcoin/bitcoin/pull/27145#discussion_r1166323841)
Done
🤔 ajtowns reviewed a pull request: "rpc: Add importmempool RPC"
(https://github.com/bitcoin/bitcoin/pull/27460#pullrequestreview-1384593990)
Approach ACK
(https://github.com/bitcoin/bitcoin/pull/27460#pullrequestreview-1384593990)
Approach ACK
💬 ajtowns commented on pull request "rpc: Add importmempool RPC":
(https://github.com/bitcoin/bitcoin/pull/27460#discussion_r1166274761)
Maybe reference `prioritisetransaction` here explicitly?
(https://github.com/bitcoin/bitcoin/pull/27460#discussion_r1166274761)
Maybe reference `prioritisetransaction` here explicitly?
💬 ajtowns commented on pull request "rpc: Add importmempool RPC":
(https://github.com/bitcoin/bitcoin/pull/27460#discussion_r1166325126)
Should this also include the ability to override the (added to the mempool) `nTime` data with the current time (or an explicitly nominated time)?
Having a child transaction with an earlier `nTime` than its parent transaction might be confusing. I think the only way that can currently happen is with system/mock time shenanigans.
(https://github.com/bitcoin/bitcoin/pull/27460#discussion_r1166325126)
Should this also include the ability to override the (added to the mempool) `nTime` data with the current time (or an explicitly nominated time)?
Having a child transaction with an earlier `nTime` than its parent transaction might be confusing. I think the only way that can currently happen is with system/mock time shenanigans.
💬 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?
(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?
(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?
(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.
(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?
(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."`?
(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`
(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.
(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???")
(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)
(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
(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
...
(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
(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.
(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
...
(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
...