💬 furszy commented on pull request "sqlite: Disallow writing from multiple `SQLiteBatch`s":
(https://github.com/bitcoin/bitcoin/pull/29112#discussion_r1481412610)
This test was originally generic and it was moved to sqlite-only because of a bdb bug, see https://github.com/bitcoin/bitcoin/pull/29112#pullrequestreview-1795448339.
(https://github.com/bitcoin/bitcoin/pull/29112#discussion_r1481412610)
This test was originally generic and it was moved to sqlite-only because of a bdb bug, see https://github.com/bitcoin/bitcoin/pull/29112#pullrequestreview-1795448339.
💬 fjahr commented on pull request "test, assumeutxo: Add test to ensure failure when mempool not empty":
(https://github.com/bitcoin/bitcoin/pull/29394#discussion_r1481442396)
nit: Could remove this log line
(https://github.com/bitcoin/bitcoin/pull/29394#discussion_r1481442396)
nit: Could remove this log line
📝 kevkevinpal opened a pull request: "mempool: Loading progress added for dumping mempool transactions to disk"
(https://github.com/bitcoin/bitcoin/pull/29402)
Sometimes when shutting off bitcoind it can take a while to dump the mempool transaction onto the disk so
this change adds additional logging to the `DumpMempool` method in `kernel/mempool_persist.cpp`
Similar to https://github.com/bitcoin/bitcoin/pull/29227 this
- adds a maximum of 10 new log lines
- logs the progress for every 10%
This is in response to https://github.com/bitcoin/bitcoin/pull/29227#issuecomment-1893375082
The logs will now look like this
```
2024-02-07T13:03:5
...
(https://github.com/bitcoin/bitcoin/pull/29402)
Sometimes when shutting off bitcoind it can take a while to dump the mempool transaction onto the disk so
this change adds additional logging to the `DumpMempool` method in `kernel/mempool_persist.cpp`
Similar to https://github.com/bitcoin/bitcoin/pull/29227 this
- adds a maximum of 10 new log lines
- logs the progress for every 10%
This is in response to https://github.com/bitcoin/bitcoin/pull/29227#issuecomment-1893375082
The logs will now look like this
```
2024-02-07T13:03:5
...
💬 maflcko commented on pull request "mempool: Loading progress added for dumping mempool transactions to disk":
(https://github.com/bitcoin/bitcoin/pull/29402#issuecomment-1932034222)
Why is this needed? A progress log for something that takes less than a second does not sound useful
(https://github.com/bitcoin/bitcoin/pull/29402#issuecomment-1932034222)
Why is this needed? A progress log for something that takes less than a second does not sound useful
💬 kevkevinpal commented on pull request "mempool: Loading progress added for dumping mempool transactions to disk":
(https://github.com/bitcoin/bitcoin/pull/29402#issuecomment-1932077013)
> Why is this needed? A progress log for something that takes less than a second does not sound useful
This is in response to this comment https://github.com/bitcoin/bitcoin/pull/29227#pullrequestreview-1817064507
the logs I posted in the description are from testnet I can run this on mainnet to get the time it takes on shutdown there
(https://github.com/bitcoin/bitcoin/pull/29402#issuecomment-1932077013)
> Why is this needed? A progress log for something that takes less than a second does not sound useful
This is in response to this comment https://github.com/bitcoin/bitcoin/pull/29227#pullrequestreview-1817064507
the logs I posted in the description are from testnet I can run this on mainnet to get the time it takes on shutdown there
📝 furszy opened a pull request: "wallet: batch erase procedures and improve 'EraseRecords' performance"
(https://github.com/bitcoin/bitcoin/pull/29403)
The first two commits are coming from #26836. They are required for these changes.
Seeks to optimize and simplify `WalletBatch::EraseRecords`. Currently, this process opens a cursor to iterate over the entire database, searching for records that match the type prefixes, to then call the `WalletBatch::Erase` function for each of the matching records.
This PR rewrites this 40-line manual process into a single line; instead of performing all of those actions manually, we can simply utilize the
...
(https://github.com/bitcoin/bitcoin/pull/29403)
The first two commits are coming from #26836. They are required for these changes.
Seeks to optimize and simplify `WalletBatch::EraseRecords`. Currently, this process opens a cursor to iterate over the entire database, searching for records that match the type prefixes, to then call the `WalletBatch::Erase` function for each of the matching records.
This PR rewrites this 40-line manual process into a single line; instead of performing all of those actions manually, we can simply utilize the
...
💬 brunoerg commented on pull request "rpc: getdescriptorinfo also returns normalized descriptor":
(https://github.com/bitcoin/bitcoin/pull/29396#issuecomment-1932083218)
```
test 2024-02-07T08:26:22.085000Z TestFramework (ERROR): JSONRPC error
Traceback (most recent call last):
File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 131, in main
self.run_test()
File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/wal
...
(https://github.com/bitcoin/bitcoin/pull/29396#issuecomment-1932083218)
```
test 2024-02-07T08:26:22.085000Z TestFramework (ERROR): JSONRPC error
Traceback (most recent call last):
File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 131, in main
self.run_test()
File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/wal
...
💬 maflcko commented on pull request "mempool: Loading progress added for dumping mempool transactions to disk":
(https://github.com/bitcoin/bitcoin/pull/29402#issuecomment-1932088394)
The delay is probably from leveldb, but it would be good to first check
(https://github.com/bitcoin/bitcoin/pull/29402#issuecomment-1932088394)
The delay is probably from leveldb, but it would be good to first check
💬 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.