💬 l0rinc commented on pull request "validation: do not wipe utxo cache for stats/scans/snapshots":
(https://github.com/bitcoin/bitcoin/pull/33680#discussion_r2483930960)
Good point, changed to `false`, added you as coauthor, thanks.
(https://github.com/bitcoin/bitcoin/pull/33680#discussion_r2483930960)
Good point, changed to `false`, added you as coauthor, thanks.
💬 l0rinc commented on pull request "validation: do not wipe utxo cache for stats/scans/snapshots":
(https://github.com/bitcoin/bitcoin/pull/33680#issuecomment-3476858887)
Thanks @optout21 & @cedwies, split the change to 2 commits, one just renames the old value and adjusts the docs and scripts, the second introduces the new value.
(https://github.com/bitcoin/bitcoin/pull/33680#issuecomment-3476858887)
Thanks @optout21 & @cedwies, split the change to 2 commits, one just renames the old value and adjusts the docs and scripts, the second introduces the new value.
💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads >20% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#issuecomment-3476888214)
Rebased to test behavior with #31645.
Some other touch-ups include:
- Use `const COutPoint&` in `Input` struct instead of `vin` and `vtx` indexes
- Cleanup shared vectors and pointers at the end of loop
- Refactor inner work loop to do fewer existence checks at the expense of some duplicated code
- Use `std::atomic_usize_t` instead of `std::atomic<usize_t>`
- Use `GetPossiblySpentCoinFromCache` in tests and fuzz harness for better clarity and correctness
(https://github.com/bitcoin/bitcoin/pull/31132#issuecomment-3476888214)
Rebased to test behavior with #31645.
Some other touch-ups include:
- Use `const COutPoint&` in `Input` struct instead of `vin` and `vtx` indexes
- Cleanup shared vectors and pointers at the end of loop
- Refactor inner work loop to do fewer existence checks at the expense of some duplicated code
- Use `std::atomic_usize_t` instead of `std::atomic<usize_t>`
- Use `GetPossiblySpentCoinFromCache` in tests and fuzz harness for better clarity and correctness
💬 l0rinc commented on pull request "[IBD] coins: reduce lookups in dbcache layer propagation":
(https://github.com/bitcoin/bitcoin/pull/33602#discussion_r2483964717)
You mean something like:
```C++
if (it->second.IsFresh() && it->second.coin.IsSpent() && !cacheCoins.contains(it->first)) {
continue; // nothing to propagate
}
```
Wouldn't that defeat the purpose, doing the check twice? Or you mean that we don't have to check `cacheCoins` for presence and should update `coins_tests/ccoins_write` to not have the false values?
That's why I've added a TODO instead, I want this PR to be simpler and do the cleanup separately. What
...
(https://github.com/bitcoin/bitcoin/pull/33602#discussion_r2483964717)
You mean something like:
```C++
if (it->second.IsFresh() && it->second.coin.IsSpent() && !cacheCoins.contains(it->first)) {
continue; // nothing to propagate
}
```
Wouldn't that defeat the purpose, doing the check twice? Or you mean that we don't have to check `cacheCoins` for presence and should update `coins_tests/ccoins_write` to not have the false values?
That's why I've added a TODO instead, I want this PR to be simpler and do the cleanup separately. What
...
💬 l0rinc commented on pull request "[IBD] coins: reduce lookups in dbcache layer propagation":
(https://github.com/bitcoin/bitcoin/pull/33602#discussion_r2483964762)
Changed it to `will_reuse_cache` - I didn't think a lot about the name, I still hope we can completely get rid of the reallocation, I don't think it makes a lot of sense this way. But let's turn it off at least of the cases where it's obviously wrong.
> I also think it would make sense to have a default true.
We have a lot from both, it's not obvious why that would be default. I'd rather prefer explicit in this case.
(https://github.com/bitcoin/bitcoin/pull/33602#discussion_r2483964762)
Changed it to `will_reuse_cache` - I didn't think a lot about the name, I still hope we can completely get rid of the reallocation, I don't think it makes a lot of sense this way. But let's turn it off at least of the cases where it's obviously wrong.
> I also think it would make sense to have a default true.
We have a lot from both, it's not obvious why that would be default. I'd rather prefer explicit in this case.
💬 andrewtoth commented on pull request "[IBD] coins: reduce lookups in dbcache layer propagation":
(https://github.com/bitcoin/bitcoin/pull/33602#discussion_r2483969075)
This check
```
if (it->second.IsFresh() && it->second.coin.IsSpent()) {
```
Does not depend on `itUs` or `cacheCoins`, right?
So we can move it up to
```
if (!it->second.IsDirty() || ((it->second.IsFresh() && it->second.coin.IsSpent())) { // TODO a cursor can only contain dirty entries
```
Right?
(https://github.com/bitcoin/bitcoin/pull/33602#discussion_r2483969075)
This check
```
if (it->second.IsFresh() && it->second.coin.IsSpent()) {
```
Does not depend on `itUs` or `cacheCoins`, right?
So we can move it up to
```
if (!it->second.IsDirty() || ((it->second.IsFresh() && it->second.coin.IsSpent())) { // TODO a cursor can only contain dirty entries
```
Right?
💬 andrewtoth commented on pull request "[IBD] coins: reduce lookups in dbcache layer propagation":
(https://github.com/bitcoin/bitcoin/pull/33602#discussion_r2483971855)
> I still hope we can completely get rid of the reallocation, I don't think it makes a lot of sense this way.
Well, we would need to get rid of the custom allocator. I think that would be a big perf hit...
> We have multiple cases for both, it's not obvious why any should be default.
Default IMO should be we can reuse, because it doesn't break if we accidentally reuse when we end up throwing it away anyways. The reverse is not true. The non-reuse case should be the only one where we hav
...
(https://github.com/bitcoin/bitcoin/pull/33602#discussion_r2483971855)
> I still hope we can completely get rid of the reallocation, I don't think it makes a lot of sense this way.
Well, we would need to get rid of the custom allocator. I think that would be a big perf hit...
> We have multiple cases for both, it's not obvious why any should be default.
Default IMO should be we can reuse, because it doesn't break if we accidentally reuse when we end up throwing it away anyways. The reverse is not true. The non-reuse case should be the only one where we hav
...
💬 l0rinc commented on pull request "[IBD] coins: reduce lookups in dbcache layer propagation":
(https://github.com/bitcoin/bitcoin/pull/33602#discussion_r2483974664)
> Does not depend on itUs or cacheCoins, right?
Not exactly, we're only executing that branch when `cacheCoins` was missing `it->first`.
Doing your suggested:
```
if (!it->second.IsDirty() || (it->second.IsFresh() && it->second.coin.IsSpent())) { // TODO a cursor can only contain dirty entries
continue;
}
```
results in the following test failures
```bash
cmake -B build -DCMAKE_BUILD_TYPE=Debug && cmake --build build && ./build/bin/test_bitcoin --run_test=coins_tests
test/coi
...
(https://github.com/bitcoin/bitcoin/pull/33602#discussion_r2483974664)
> Does not depend on itUs or cacheCoins, right?
Not exactly, we're only executing that branch when `cacheCoins` was missing `it->first`.
Doing your suggested:
```
if (!it->second.IsDirty() || (it->second.IsFresh() && it->second.coin.IsSpent())) { // TODO a cursor can only contain dirty entries
continue;
}
```
results in the following test failures
```bash
cmake -B build -DCMAKE_BUILD_TYPE=Debug && cmake --build build && ./build/bin/test_bitcoin --run_test=coins_tests
test/coi
...
💬 l0rinc commented on pull request "[IBD] coins: reduce lookups in dbcache layer propagation":
(https://github.com/bitcoin/bitcoin/pull/33602#discussion_r2483975804)
> we would need to get rid of the custom allocator
I'm not so sure, I'd have different dbcache sizes during IBD and only reallocate after it finishes, since we don't actually need to narrow the cache size during IBD. After your input fetcher change we may never need to narrow it. We'll see, let's discuss that separately.
> The non-reuse case should be the only one where we have to be explicit.
I explicitly wanted to avoid that, but let me know if you insist, I don't mind that much.
(https://github.com/bitcoin/bitcoin/pull/33602#discussion_r2483975804)
> we would need to get rid of the custom allocator
I'm not so sure, I'd have different dbcache sizes during IBD and only reallocate after it finishes, since we don't actually need to narrow the cache size during IBD. After your input fetcher change we may never need to narrow it. We'll see, let's discuss that separately.
> The non-reuse case should be the only one where we have to be explicit.
I explicitly wanted to avoid that, but let me know if you insist, I don't mind that much.
💬 oren-z0 commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2483977816)
Oh, now I get it.
So the question is why `tx_verbosity` was required in the first place even when the data-format was binary or hex (where it is ignored).
Consider splitting this into two commits, one to make `tx_verbosity` optional, and another to add the new blockpart feature.
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2483977816)
Oh, now I get it.
So the question is why `tx_verbosity` was required in the first place even when the data-format was binary or hex (where it is ignored).
Consider splitting this into two commits, one to make `tx_verbosity` optional, and another to add the new blockpart feature.
💬 andrewtoth commented on pull request "[IBD] coins: reduce lookups in dbcache layer propagation":
(https://github.com/bitcoin/bitcoin/pull/33602#discussion_r2483977917)
> I explicitly wanted to avoid that, but let me know if you insist, I don't mind that much.
Don't you think it's safer to have the case that won't break either way be default, and then the performant case that can break if you pick the wrong option be explicit?
(https://github.com/bitcoin/bitcoin/pull/33602#discussion_r2483977917)
> I explicitly wanted to avoid that, but let me know if you insist, I don't mind that much.
Don't you think it's safer to have the case that won't break either way be default, and then the performant case that can break if you pick the wrong option be explicit?
💬 l0rinc commented on pull request "[IBD] coins: reduce lookups in dbcache layer propagation":
(https://github.com/bitcoin/bitcoin/pull/33602#discussion_r2483978998)
Maybe, not sure, but I took your advice and pushed.
(https://github.com/bitcoin/bitcoin/pull/33602#discussion_r2483978998)
Maybe, not sure, but I took your advice and pushed.
💬 andrewtoth commented on pull request "[IBD] coins: reduce lookups in dbcache layer propagation":
(https://github.com/bitcoin/bitcoin/pull/33602#discussion_r2483980214)
Ahh right, we handle this case even though it can't be hit in production.
I suppose it's valuable for regression testing, so we throw instead of silently allowing this.
(https://github.com/bitcoin/bitcoin/pull/33602#discussion_r2483980214)
Ahh right, we handle this case even though it can't be hit in production.
I suppose it's valuable for regression testing, so we throw instead of silently allowing this.
🤔 stringintech reviewed a pull request: "kernel: Introduce C header API"
(https://github.com/bitcoin/bitcoin/pull/30595#pullrequestreview-3407306489)
Comments on last 8 commits (dcb8d495 through e9f14a07).
(https://github.com/bitcoin/bitcoin/pull/30595#pullrequestreview-3407306489)
Comments on last 8 commits (dcb8d495 through e9f14a07).
💬 stringintech commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2483514108)
nit
```suggestion
if (m_expected_valid_block.has_value()) {
auto ser_block{block.ToBytes()};
check_equal(m_expected_valid_block.value(), ser_block);
}
```
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2483514108)
nit
```suggestion
if (m_expected_valid_block.has_value()) {
auto ser_block{block.ToBytes()};
check_equal(m_expected_valid_block.value(), ser_block);
}
```
💬 stringintech commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2483940512)
nit
```suggestion
Logger logger{std::make_unique<KernelLog>()};
```
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2483940512)
nit
```suggestion
Logger logger{std::make_unique<KernelLog>()};
```
💬 stringintech commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2483695548)
If the chain reference we get here remains a valid pointer to the best chain even in the case of reorgs (i'm ignoring the more unlikely case where the active chainstate could change), maybe the doc can be a bit more clear:
```cpp
/**
* @brief Returns the best known currently active chain. Its lifetime is
* dependent on the chainstate manager. It can be thought of as a view on a
* vector of block tree entries that form the best chain. The returned chain
* reference always points to the
...
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2483695548)
If the chain reference we get here remains a valid pointer to the best chain even in the case of reorgs (i'm ignoring the more unlikely case where the active chainstate could change), maybe the doc can be a bit more clear:
```cpp
/**
* @brief Returns the best known currently active chain. Its lifetime is
* dependent on the chainstate manager. It can be thought of as a view on a
* vector of block tree entries that form the best chain. The returned chain
* reference always points to the
...
💬 stringintech commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2483939969)
nit
```suggestion
Logger logger{std::make_unique<TestLog>()};
```
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2483939969)
nit
```suggestion
Logger logger{std::make_unique<TestLog>()};
```
💬 stringintech commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2483614770)
If it is possible to have another thread extending/changing the best chain, then i think these three functions should hold `cs_main`. For example all three are calling `size()` on the underlying chain vector.
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2483614770)
If it is possible to have another thread extending/changing the best chain, then i think these three functions should hold `cs_main`. For example all three are calling `size()` on the underlying chain vector.
💬 stringintech commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2483701019)
The test section 911-934 could become a bit easier to follow if it is broken into multiple parts (just some newlines might be enough):
<details>
<summary>diff</summary>
```diff
diff --git a/src/test/kernel/test_kernel.cpp b/src/test/kernel/test_kernel.cpp
index e2c14b57eb..f78f346711 100644
--- a/src/test/kernel/test_kernel.cpp
+++ b/src/test/kernel/test_kernel.cpp
@@ -908,25 +908,34 @@ BOOST_AUTO_TEST_CASE(btck_chainman_regtest_tests)
}
}
+ // Read spent outputs
...
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2483701019)
The test section 911-934 could become a bit easier to follow if it is broken into multiple parts (just some newlines might be enough):
<details>
<summary>diff</summary>
```diff
diff --git a/src/test/kernel/test_kernel.cpp b/src/test/kernel/test_kernel.cpp
index e2c14b57eb..f78f346711 100644
--- a/src/test/kernel/test_kernel.cpp
+++ b/src/test/kernel/test_kernel.cpp
@@ -908,25 +908,34 @@ BOOST_AUTO_TEST_CASE(btck_chainman_regtest_tests)
}
}
+ // Read spent outputs
...
💬 stringintech commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2483756743)
Not sure of this but could we return an optional here instead of throwing? (for the same reason that the assertion was removed from `btck_chain_get_by_height()` impl)
Also for `GetBlockTreeEntry()` that implicitly throws in `BlockTreeEntry` ctor now.
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2483756743)
Not sure of this but could we return an optional here instead of throwing? (for the same reason that the assertion was removed from `btck_chain_get_by_height()` impl)
Also for `GetBlockTreeEntry()` that implicitly throws in `BlockTreeEntry` ctor now.