💬 fjahr commented on pull request "index: Check all necessary block data is available before starting to sync":
(https://github.com/bitcoin/bitcoin/pull/29770#issuecomment-2764580300)
I am making one more effort to pick this up see if it can get the necessary support. Upon re-reading our conversation @furszy I have removed the `chain.h` dependency from the indices so that it's a bit closer to what #24230 is planning to do. I looked at it as well to see if there are commits available to pick to add a options object but that didn't seem to be the case and #24230 is also still in draft mode so unsure how valuable it would be if I pick some small piece of code out of it. The code
...
(https://github.com/bitcoin/bitcoin/pull/29770#issuecomment-2764580300)
I am making one more effort to pick this up see if it can get the necessary support. Upon re-reading our conversation @furszy I have removed the `chain.h` dependency from the indices so that it's a bit closer to what #24230 is planning to do. I looked at it as well to see if there are commits available to pick to add a options object but that didn't seem to be the case and #24230 is also still in draft mode so unsure how valuable it would be if I pick some small piece of code out of it. The code
...
💬 l0rinc commented on pull request "Use number of dirty cache entries in flush warnings/logs":
(https://github.com/bitcoin/bitcoin/pull/31703#discussion_r2020168731)
This could also be useful to avoid the needless DB_HEAD_BLOCKS erase and rewrite for empty batches.
(https://github.com/bitcoin/bitcoin/pull/31703#discussion_r2020168731)
This could also be useful to avoid the needless DB_HEAD_BLOCKS erase and rewrite for empty batches.
📝 l0rinc converted_to_draft a pull request: "[IBD] flush UTXOs in bigger batches"
(https://github.com/bitcoin/bitcoin/pull/31645)
This change is part of [[IBD] - Tracking PR for speeding up Initial Block Download](https://github.com/bitcoin/bitcoin/pull/32043)
### Summary
The final UTXO set is [written to disk in batches](https://github.com/bitcoin/bitcoin/blob/master/src/txdb.cpp#L130-L133) to avoid a gigantic spike at flush time.
There is already a [-dbbatchsize](https://github.com/bitcoin/bitcoin/blob/master/src/init.cpp#L490) config option to change this value, this PR adjusts the default only.
By increasing th
...
(https://github.com/bitcoin/bitcoin/pull/31645)
This change is part of [[IBD] - Tracking PR for speeding up Initial Block Download](https://github.com/bitcoin/bitcoin/pull/32043)
### Summary
The final UTXO set is [written to disk in batches](https://github.com/bitcoin/bitcoin/blob/master/src/txdb.cpp#L130-L133) to avoid a gigantic spike at flush time.
There is already a [-dbbatchsize](https://github.com/bitcoin/bitcoin/blob/master/src/init.cpp#L490) config option to change this value, this PR adjusts the default only.
By increasing th
...
💬 hebasto commented on issue "oss-fuzz build fails":
(https://github.com/bitcoin/bitcoin/issues/32167#issuecomment-2764589279)
Reverting https://github.com/bitcoin/bitcoin/pull/31849 fixes the issue.
cc @fanquake
(https://github.com/bitcoin/bitcoin/issues/32167#issuecomment-2764589279)
Reverting https://github.com/bitcoin/bitcoin/pull/31849 fixes the issue.
cc @fanquake
💬 bigspider commented on pull request "OP_CHECKCONTRACTVERIFY":
(https://github.com/bitcoin/bitcoin/pull/32080#issuecomment-2764601895)
I added a workaround to disable the two failing tests from the `script_assets_test`; those tests do indeed assume the `OP_SUCCESS` behaviour of the `0xBB` opcode.
(https://github.com/bitcoin/bitcoin/pull/32080#issuecomment-2764601895)
I added a workaround to disable the two failing tests from the `script_assets_test`; those tests do indeed assume the `OP_SUCCESS` behaviour of the `0xBB` opcode.
💬 l0rinc commented on pull request "[IBD] flush UTXOs in bigger batches":
(https://github.com/bitcoin/bitcoin/pull/31645#issuecomment-2764621216)
I've experimented a bit with changing the fixed write batch size to a dynamic value that scales with dbcache size, basically something like:
```C++
static int64_t GetDefaultDbBatchSize(int64_t dbcache_bytes) {
return std::min<int64_t>(
MAX_DB_CACHE_BATCH, // 512 MiB upper limit
std::max<int64_t>(
MIN_DB_CACHE, // 4 MiB lower limit
dbcache_bytes * DEFAULT_KERNEL_CACHE_BATCH_SIZE / DEFAULT_KERNEL_CACHE
)
);
}
```
Corresponding
...
(https://github.com/bitcoin/bitcoin/pull/31645#issuecomment-2764621216)
I've experimented a bit with changing the fixed write batch size to a dynamic value that scales with dbcache size, basically something like:
```C++
static int64_t GetDefaultDbBatchSize(int64_t dbcache_bytes) {
return std::min<int64_t>(
MAX_DB_CACHE_BATCH, // 512 MiB upper limit
std::max<int64_t>(
MIN_DB_CACHE, // 4 MiB lower limit
dbcache_bytes * DEFAULT_KERNEL_CACHE_BATCH_SIZE / DEFAULT_KERNEL_CACHE
)
);
}
```
Corresponding
...
🤔 fjahr reviewed a pull request: "wallet, migration: Fix empty wallet crash"
(https://github.com/bitcoin/bitcoin/pull/32149#pullrequestreview-2727937771)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/32149#pullrequestreview-2727937771)
Concept ACK
💬 fjahr commented on pull request "wallet, migration: Fix empty wallet crash":
(https://github.com/bitcoin/bitcoin/pull/32149#discussion_r2020190503)
```suggestion
//! Returns true if there are any DBKeys::LEGACY_TYPES record in the wallet db
```
(https://github.com/bitcoin/bitcoin/pull/32149#discussion_r2020190503)
```suggestion
//! Returns true if there are any DBKeys::LEGACY_TYPES record in the wallet db
```
💬 fjahr commented on pull request "wallet, migration: Fix empty wallet crash":
(https://github.com/bitcoin/bitcoin/pull/32149#discussion_r2020189978)
Seems like this can be simplified (with maybe an additional comment that an empty wallet means automatic success)
```suggestion
{
// Keep the batch alive only for this call
const auto& db_batch = local_wallet->GetDatabase().MakeBatch();
success = !HasLegacyRecords(local_wallet.get(), *db_batch);
}
// Do the migration of keys and scripts for non-blank wallets, and cleanup if it fails
```
(https://github.com/bitcoin/bitcoin/pull/32149#discussion_r2020189978)
Seems like this can be simplified (with maybe an additional comment that an empty wallet means automatic success)
```suggestion
{
// Keep the batch alive only for this call
const auto& db_batch = local_wallet->GetDatabase().MakeBatch();
success = !HasLegacyRecords(local_wallet.get(), *db_batch);
}
// Do the migration of keys and scripts for non-blank wallets, and cleanup if it fails
```
💬 fjahr commented on pull request "wallet, migration: Fix empty wallet crash":
(https://github.com/bitcoin/bitcoin/pull/32149#discussion_r2020190409)
nit: I find this comment a bit redundant, especially in the tests. If you rename `db_batch` to `temp_batch` IMO it could be removed.
(https://github.com/bitcoin/bitcoin/pull/32149#discussion_r2020190409)
nit: I find this comment a bit redundant, especially in the tests. If you rename `db_batch` to `temp_batch` IMO it could be removed.
💬 EthanHeilman commented on pull request "tests: improves tapscript unit tests":
(https://github.com/bitcoin/bitcoin/pull/31640#issuecomment-2764707199)
> could you squash commits?
@instagibbs
Squashed
(https://github.com/bitcoin/bitcoin/pull/31640#issuecomment-2764707199)
> could you squash commits?
@instagibbs
Squashed
💬 l0rinc commented on pull request "[IBD] batch block reads/writes during `AutoFile` serialization":
(https://github.com/bitcoin/bitcoin/pull/31551#issuecomment-2764716714)
> It seems odd that using exact block sizes (this pull initially) passes, whereas using a 1<<20 approximation fails due to OOM
Yes, I thought of the same, but the other PR had the same issue. I'm not familiar enough with the intricacies of `ASan + LSan + UBSan + integer, no depends, USDT` to have a good enough explanation - besides the instrumentation causing it to overflow.
> What is the memory usage of the CI before and after?
I have measured a valgrind/massif IBD until the be benchma
...
(https://github.com/bitcoin/bitcoin/pull/31551#issuecomment-2764716714)
> It seems odd that using exact block sizes (this pull initially) passes, whereas using a 1<<20 approximation fails due to OOM
Yes, I thought of the same, but the other PR had the same issue. I'm not familiar enough with the intricacies of `ASan + LSan + UBSan + integer, no depends, USDT` to have a good enough explanation - besides the instrumentation causing it to overflow.
> What is the memory usage of the CI before and after?
I have measured a valgrind/massif IBD until the be benchma
...
💬 l0rinc commented on pull request "refactor: Enforce readability-avoid-const-params-in-decls":
(https://github.com/bitcoin/bitcoin/pull/31650#discussion_r2020242074)
> I can understand the confusion
I saw [both were changed here](https://github.com/bitcoin/bitcoin/pull/31650/commits/fa8e57951abc5c737ab68694b8f6aa77551faa6b#r2017450776), I think it's a good change, I like consistency and though a few other similar ones could be added here.
Not sure why you'd want to close the PR because of my comments, if they're not useful, you can explain it to me or wait on other reviewers.
Feel free to resolve my comments.
(https://github.com/bitcoin/bitcoin/pull/31650#discussion_r2020242074)
> I can understand the confusion
I saw [both were changed here](https://github.com/bitcoin/bitcoin/pull/31650/commits/fa8e57951abc5c737ab68694b8f6aa77551faa6b#r2017450776), I think it's a good change, I like consistency and though a few other similar ones could be added here.
Not sure why you'd want to close the PR because of my comments, if they're not useful, you can explain it to me or wait on other reviewers.
Feel free to resolve my comments.
💬 ajtowns commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2020250395)
> A downside here is that the "linearization" for this fat singletons cluster would be O(n log n) to update on every operation involving a singleton, which may be substantial if there are 1000s of singletons?
I don't think that's true? You'd just store the linearisation as part of the special cluster implementation as a std::set by feerate order, for O(lg n) modification and O(n) reporting? I guess it would be better to call that class something other than "cluster", since you'd probably want
...
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2020250395)
> A downside here is that the "linearization" for this fat singletons cluster would be O(n log n) to update on every operation involving a singleton, which may be substantial if there are 1000s of singletons?
I don't think that's true? You'd just store the linearisation as part of the special cluster implementation as a std::set by feerate order, for O(lg n) modification and O(n) reporting? I guess it would be better to call that class something other than "cluster", since you'd probably want
...
💬 fjahr commented on pull request "index: Fix coinstats overflow and introduce index versioning":
(https://github.com/bitcoin/bitcoin/pull/30469#issuecomment-2764743201)
Picking this up again after this has gone a bit quiet for a while. After having a brief conversation about it with @mzumsande at CoreDev I took another look at his migration idea. Initially I was a bit undecided honestly but I now think that it's reasonable to try this and I would like to open this up for additional approach feedback from other reviewers. Aside from a rebase this has now cherry-picked @mzumsande 's draft commit with minor changes by me and I adapted the tests to check the new be
...
(https://github.com/bitcoin/bitcoin/pull/30469#issuecomment-2764743201)
Picking this up again after this has gone a bit quiet for a while. After having a brief conversation about it with @mzumsande at CoreDev I took another look at his migration idea. Initially I was a bit undecided honestly but I now think that it's reasonable to try this and I would like to open this up for additional approach feedback from other reviewers. Aside from a rebase this has now cherry-picked @mzumsande 's draft commit with minor changes by me and I adapted the tests to check the new be
...
✅ tnndbtc closed a pull request: "miniscript: fixes #29098 by only use first k valid signatures"
(https://github.com/bitcoin/bitcoin/pull/31719)
(https://github.com/bitcoin/bitcoin/pull/31719)
📝 tnndbtc reopened a pull request: "miniscript: fixes #29098 by only use first k valid signatures"
(https://github.com/bitcoin/bitcoin/pull/31719)
In issue #29098 a recommendation is not to use "best (smallest) set of k signatures". So, this effort is to fall back to the original algorithm which only use the first k available signatures for satisfying a k-of-n multisig. Otherwise, there will be timeout in unit test when we have 999-of-999 use case.
Profiling has been done on Mac to confirm the most time consuming function is in internal::InputResult ProduceInput.
Following tests will hit the affected code:
- ctest --test-dir bui
...
(https://github.com/bitcoin/bitcoin/pull/31719)
In issue #29098 a recommendation is not to use "best (smallest) set of k signatures". So, this effort is to fall back to the original algorithm which only use the first k available signatures for satisfying a k-of-n multisig. Otherwise, there will be timeout in unit test when we have 999-of-999 use case.
Profiling has been done on Mac to confirm the most time consuming function is in internal::InputResult ProduceInput.
Following tests will hit the affected code:
- ctest --test-dir bui
...
⚠️ 69690000 opened an issue: "Tttbbb"
(https://github.com/bitcoin/bitcoin/issues/32169)
### Motivation
Tttbbb
### Possible solution
_No response_
### Useful Skills
* Compiling Bitcoin Core from source
* Running the C++ unit tests and the Python functional tests
* ...
### Guidance for new contributors
Want to work on this issue?
For guidance on contributing, please read [CONTRIBUTING.md](https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md) before opening your pull request.
(https://github.com/bitcoin/bitcoin/issues/32169)
### Motivation
Tttbbb
### Possible solution
_No response_
### Useful Skills
* Compiling Bitcoin Core from source
* Running the C++ unit tests and the Python functional tests
* ...
### Guidance for new contributors
Want to work on this issue?
For guidance on contributing, please read [CONTRIBUTING.md](https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md) before opening your pull request.
⚠️ 69690000 opened an issue: "Tttbbb"
(https://github.com/bitcoin/bitcoin/issues/32170)
### Motivation
apr-bc14855e34295b96f158e8c62a2b11cd : ibq1wtpQd4kxKB6LkKmfe0pDN3euGdZS
### Possible solution
_No response_
### Useful Skills
* Compiling Bitcoin Core from source
* Running the C++ unit tests and the Python functional tests
* ...
### Guidance for new contributors
Want to work on this issue?
For guidance on contributing, please read [CONTRIBUTING.md](https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md) before opening your pull request.
(https://github.com/bitcoin/bitcoin/issues/32170)
### Motivation
apr-bc14855e34295b96f158e8c62a2b11cd : ibq1wtpQd4kxKB6LkKmfe0pDN3euGdZS
### Possible solution
_No response_
### Useful Skills
* Compiling Bitcoin Core from source
* Running the C++ unit tests and the Python functional tests
* ...
### Guidance for new contributors
Want to work on this issue?
For guidance on contributing, please read [CONTRIBUTING.md](https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md) before opening your pull request.
⚠️ 69690000 opened an issue: "Tttbbb"
(https://github.com/bitcoin/bitcoin/issues/32171)
### Motivation
[export-0x71c7656ec7ab88b098defb751b7401b5f6d8976f.docx](https://github.com/user-attachments/files/19527909/export-0x71c7656ec7ab88b098defb751b7401b5f6d8976f.docx)
### Possible solution
_No response_
### Useful Skills
* Compiling Bitcoin Core from source
* Running the C++ unit tests and the Python functional tests
* ...
### Guidance for new contributors
Want to work on this issue?
For guidance on contributing, please read [CONTRIBUTING.md](https://github.com/bitcoin/bitco
...
(https://github.com/bitcoin/bitcoin/issues/32171)
### Motivation
[export-0x71c7656ec7ab88b098defb751b7401b5f6d8976f.docx](https://github.com/user-attachments/files/19527909/export-0x71c7656ec7ab88b098defb751b7401b5f6d8976f.docx)
### Possible solution
_No response_
### Useful Skills
* Compiling Bitcoin Core from source
* Running the C++ unit tests and the Python functional tests
* ...
### Guidance for new contributors
Want to work on this issue?
For guidance on contributing, please read [CONTRIBUTING.md](https://github.com/bitcoin/bitco
...
✅ fanquake closed an issue: "Tttbbb"
(https://github.com/bitcoin/bitcoin/issues/32169)
(https://github.com/bitcoin/bitcoin/issues/32169)