💬 maflcko commented on pull request "wallet: batch erase procedures and improve 'EraseRecords' performance":
(https://github.com/bitcoin/bitcoin/pull/29403#discussion_r1481506563)
nit, feel free to ignore: Any reason to add `__func__` to this log, when none of the other logs have it here? If someone is interested in the exact source location, they can enable the corresponding log option. If this is needed for context, it could make more sense to clarify the log message itself, so that it is unique and clear without requiring the source code as context.
(https://github.com/bitcoin/bitcoin/pull/29403#discussion_r1481506563)
nit, feel free to ignore: Any reason to add `__func__` to this log, when none of the other logs have it here? If someone is interested in the exact source location, they can enable the corresponding log option. If this is needed for context, it could make more sense to clarify the log message itself, so that it is unique and clear without requiring the source code as context.
💬 furszy commented on pull request "wallet: batch erase procedures and improve 'EraseRecords' performance":
(https://github.com/bitcoin/bitcoin/pull/29403#discussion_r1481525685)
> Any reason to add `__func__` to this log, when none of the other logs have it here?
Not really. It is just a bad habit. I pulled 9a4e1f5 from #26836 because it is required for this changes. Will change it there.
(https://github.com/bitcoin/bitcoin/pull/29403#discussion_r1481525685)
> Any reason to add `__func__` to this log, when none of the other logs have it here?
Not really. It is just a bad habit. I pulled 9a4e1f5 from #26836 because it is required for this changes. Will change it there.
💬 Xaspr commented on issue "Unable to sync blockchain on laptop: ERROR: ReadBlockFromDisk: Deserialize or I/O error":
(https://github.com/bitcoin/bitcoin/issues/29255#issuecomment-1932120188)
> It would be good to check this. If you have a completely separate machine, you could try to install the "anti"-virus software there and see if the issue happens?
I don't have another windows machine, so unfortunately I'm not able to check this. I'm not sure if it would do much, because I've now gotten to IBD several times without issues, whereas before when antivirus was still active, the issues came up persistently around block height 350,000-450,000 or so.
So I didn't get the previous
...
(https://github.com/bitcoin/bitcoin/issues/29255#issuecomment-1932120188)
> It would be good to check this. If you have a completely separate machine, you could try to install the "anti"-virus software there and see if the issue happens?
I don't have another windows machine, so unfortunately I'm not able to check this. I'm not sure if it would do much, because I've now gotten to IBD several times without issues, whereas before when antivirus was still active, the issues came up persistently around block height 350,000-450,000 or so.
So I didn't get the previous
...
🤔 furszy reviewed a pull request: "wallet: batch and simplify addressbook migration process"
(https://github.com/bitcoin/bitcoin/pull/26836#pullrequestreview-1867915441)
Updated per @maflcko review in https://github.com/bitcoin/bitcoin/pull/29403#discussion_r1481506563. [Small diff](https://github.com/bitcoin/bitcoin/compare/238993ea40c1c384e9fbcf83bfcf45efc73ad1bb..4c4c62b459e5b92392a67cb369101b07fd4bad28).
(https://github.com/bitcoin/bitcoin/pull/26836#pullrequestreview-1867915441)
Updated per @maflcko review in https://github.com/bitcoin/bitcoin/pull/29403#discussion_r1481506563. [Small diff](https://github.com/bitcoin/bitcoin/compare/238993ea40c1c384e9fbcf83bfcf45efc73ad1bb..4c4c62b459e5b92392a67cb369101b07fd4bad28).
💬 furszy commented on pull request "wallet: batch erase procedures and improve 'EraseRecords' performance":
(https://github.com/bitcoin/bitcoin/pull/29403#issuecomment-1932141334)
Updated per feedback. Thanks Marko.
(https://github.com/bitcoin/bitcoin/pull/29403#issuecomment-1932141334)
Updated per feedback. Thanks Marko.
💬 theStack commented on pull request "mempool: Loading progress added for dumping mempool transactions to disk":
(https://github.com/bitcoin/bitcoin/pull/29402#issuecomment-1932187117)
> Why is this needed? A progress log for something that takes less than a second does not sound useful
I agree that logging the detailed progress in % is a bit overkill, but IMHO a single line "Dumping xyz mempool transactions to disk..." would still make sense. The "less than a second" data point seems to be closer to best-case, I have one machine here where dumping a full default-sized mempool takes more than 10 seconds.
(https://github.com/bitcoin/bitcoin/pull/29402#issuecomment-1932187117)
> Why is this needed? A progress log for something that takes less than a second does not sound useful
I agree that logging the detailed progress in % is a bit overkill, but IMHO a single line "Dumping xyz mempool transactions to disk..." would still make sense. The "less than a second" data point seems to be closer to best-case, I have one machine here where dumping a full default-sized mempool takes more than 10 seconds.
💬 maflcko commented on pull request "mempool: Loading progress added for dumping mempool transactions to disk":
(https://github.com/bitcoin/bitcoin/pull/29402#issuecomment-1932225006)
> 10 seconds.
Sure, it may take longer than one second, depending on the hardware and its size. However,
* *Loading* a mempool can take hours, depending on the hardware and its size.
* *Loading* is done in a background thread, so other stuff is going on and logged at the same time.
Shutdown should be single-threaded, so the debug log should contain the last timestamp, which is enough to derive how long it took to dump the mempool, or whether it is still ongoing. I don't expect anyone t
...
(https://github.com/bitcoin/bitcoin/pull/29402#issuecomment-1932225006)
> 10 seconds.
Sure, it may take longer than one second, depending on the hardware and its size. However,
* *Loading* a mempool can take hours, depending on the hardware and its size.
* *Loading* is done in a background thread, so other stuff is going on and logged at the same time.
Shutdown should be single-threaded, so the debug log should contain the last timestamp, which is enough to derive how long it took to dump the mempool, or whether it is still ongoing. I don't expect anyone t
...
💬 epiccurious commented on pull request "mempool: Loading progress added for dumping mempool transactions to disk":
(https://github.com/bitcoin/bitcoin/pull/29402#issuecomment-1932225974)
> logging the detailed progress in % is a bit overkill
+1 to @theStack's comment. Logging the start and end would be sufficient, no need for the 10% increments.
(https://github.com/bitcoin/bitcoin/pull/29402#issuecomment-1932225974)
> logging the detailed progress in % is a bit overkill
+1 to @theStack's comment. Logging the start and end would be sufficient, no need for the 10% increments.
🚀 fanquake merged a pull request: "test: Fix CPartialMerkleTree.nTransactions signedness"
(https://github.com/bitcoin/bitcoin/pull/29363)
(https://github.com/bitcoin/bitcoin/pull/29363)
🤔 furszy reviewed a pull request: "wallet: track mempool conflicts with wallet transactions"
(https://github.com/bitcoin/bitcoin/pull/27307#pullrequestreview-1867972899)
light review, left few comments, nothing big.
(https://github.com/bitcoin/bitcoin/pull/27307#pullrequestreview-1867972899)
light review, left few comments, nothing big.
💬 furszy commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1481573074)
In 86aaf81f8:
If you touch this again; could pull `mit->second` into its own variable to improve readability.
```c++
const auto& wtx = mit->second;
// An output is considered spent by a spending transaction,
// unless the spending transaction is conflicted or abandoned.
if (wtx.isConfirmed() || wtx.InMempool() || (wtx.isInactive() && !wtx.isAbandoned()))
return true;
```
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1481573074)
In 86aaf81f8:
If you touch this again; could pull `mit->second` into its own variable to improve readability.
```c++
const auto& wtx = mit->second;
// An output is considered spent by a spending transaction,
// unless the spending transaction is conflicted or abandoned.
if (wtx.isConfirmed() || wtx.InMempool() || (wtx.isInactive() && !wtx.isAbandoned()))
return true;
```
💬 furszy commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1481616345)
Could document why it has been replaced here. It is not immediately evident only by reading the code.
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1481616345)
Could document why it has been replaced here. It is not immediately evident only by reading the code.
💬 furszy commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1481627720)
Would be good to document why you are doing this. I guess that you are cleaning up the mempool and wallet for the next test?
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1481627720)
Would be good to document why you are doing this. I guess that you are cleaning up the mempool and wallet for the next test?
💬 furszy commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1481600177)
Would be good to explain that you are doing the `mapWallet` lookup because the wtx could have been updated. Because otherwise, this could just be a `if (_it->second == conflict_txid) continue;` right?.
Also, as you already know that the output belongs to one of the wallet's txs, could:
```c++
CWalletTx& wtx = mapWallet.at(_it->second);
if (wtx.tx->GetWitnessHash() == conflict_wtxid) continue;
```
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1481600177)
Would be good to explain that you are doing the `mapWallet` lookup because the wtx could have been updated. Because otherwise, this could just be a `if (_it->second == conflict_txid) continue;` right?.
Also, as you already know that the output belongs to one of the wallet's txs, could:
```c++
CWalletTx& wtx = mapWallet.at(_it->second);
if (wtx.tx->GetWitnessHash() == conflict_wtxid) continue;
```
💬 furszy commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1481609541)
Can remove the initial `wtx.mempool_conflicts.empty()` call and directly call `erase`.
```c++
auto try_updating_state = [&](CWalletTx& wtx) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet) {
// Remove the previously conflicting transaction from this wtx's entry
wtx.mempool_conflicts.erase(conflict_txid);
if (wtx.mempool_conflicts.empty()) {
// If this wtx has no other mempool conflicts, it is now considered inactive
if (wtx.isMempoolConflicted()) {
wtx.m_stat
...
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1481609541)
Can remove the initial `wtx.mempool_conflicts.empty()` call and directly call `erase`.
```c++
auto try_updating_state = [&](CWalletTx& wtx) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet) {
// Remove the previously conflicting transaction from this wtx's entry
wtx.mempool_conflicts.erase(conflict_txid);
if (wtx.mempool_conflicts.empty()) {
// If this wtx has no other mempool conflicts, it is now considered inactive
if (wtx.isMempoolConflicted()) {
wtx.m_stat
...
💬 furszy commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1481621542)
Would be good to document the behavior inside the code. It is not immediately evident only by reading the code.
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1481621542)
Would be good to document the behavior inside the code. It is not immediately evident only by reading the code.
💬 kevkevinpal commented on pull request "mempool: Log added for dumping mempool transactions to disk":
(https://github.com/bitcoin/bitcoin/pull/29402#issuecomment-1932261641)
force pushed [33153b0](https://github.com/bitcoin/bitcoin/pull/29402/commits/33153b0236346809c75f6f52a3000a4e18e3b385)
This removes the 10% increments and now logs a single line with the amount of mempool transactions being written to the disk
(https://github.com/bitcoin/bitcoin/pull/29402#issuecomment-1932261641)
force pushed [33153b0](https://github.com/bitcoin/bitcoin/pull/29402/commits/33153b0236346809c75f6f52a3000a4e18e3b385)
This removes the 10% increments and now logs a single line with the amount of mempool transactions being written to the disk
💬 furszy commented on pull request "wallet: batch and simplify addressbook migration process":
(https://github.com/bitcoin/bitcoin/pull/26836#issuecomment-1932273223)
Green CI. Ready to go.
(https://github.com/bitcoin/bitcoin/pull/26836#issuecomment-1932273223)
Green CI. Ready to go.
💬 furszy commented on pull request "wallet: batch erase procedures and improve 'EraseRecords' performance":
(https://github.com/bitcoin/bitcoin/pull/29403#issuecomment-1932273984)
Green CI. Ready to go.
(https://github.com/bitcoin/bitcoin/pull/29403#issuecomment-1932273984)
Green CI. Ready to go.
💬 theuni commented on issue "doc: List identifiers used from header in #if(def)":
(https://github.com/bitcoin/bitcoin/issues/26972#issuecomment-1932303445)
@sipa Thanks.
@maflcko If we should decide to list the used identifiers for each `bitcoin-config.h` include as you've suggested here (which I'm not sure is necessary, but not opposed to either), we'd need to make sure to include the file where needed in each instance.
So I believe something like the above branch would be a requirement. And since @sipa and @hebasto agree, I think it makes sense to go ahead and do that either way.
I'll open a PR for that branch with instructions to reprod
...
(https://github.com/bitcoin/bitcoin/issues/26972#issuecomment-1932303445)
@sipa Thanks.
@maflcko If we should decide to list the used identifiers for each `bitcoin-config.h` include as you've suggested here (which I'm not sure is necessary, but not opposed to either), we'd need to make sure to include the file where needed in each instance.
So I believe something like the above branch would be a requirement. And since @sipa and @hebasto agree, I think it makes sense to go ahead and do that either way.
I'll open a PR for that branch with instructions to reprod
...