💬 ryanofsky commented on pull request "index: make startup more efficient":
(https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1253898394)
In commit "make GetFirstStoredBlock assert that 'start_block' always has data" (aad83e489a9f515a1da36acd3e99f35b1a0da53c)

The `pruneblockchain` RPC doesn't consider it an error to prune a block that's already been pruned. It just makes a best effort to prune what it can, and then it returns the last height of block without transaction data.

So I think it would be more consistent to not make this an error, and just do:

```c++
PruneBlockFilesManual(active_chainstate, height);
return blo
...

💬 ryanofsky commented on pull request "index: make startup more efficient":
(https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1253906502)
In commit "make GetFirstStoredBlock assert that 'start_block' always has data" (aad83e489a9f515a1da36acd3e99f35b1a0da53c)

This preserves current behavior, but it is inconsistent with the documentation which says the field is be set to "height of the last block pruned, plus one". To make the code actually consistent with the documentation, this should be:

```c++
obj.pushKV("pruneheight", has_tip_data ? chainman.m_blockman.GetFirstStoredBlock(tip)->nHeight : tip.nHeight + 1);
```

Altern
...

💬 ryanofsky commented on pull request "index: make startup more efficient":
(https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1253746784)
In commit "make GetFirstStoredBlock assert that 'start_block' always has data" (aad83e489a9f515a1da36acd3e99f35b1a0da53c)

It seems like there is a slight change in behavior here. Previously if the tip did not have data, but all blocks before it did have data there would not be a prune violation. Now there will be prune violation.

This is probably a good thing. I don't think this case actually needs to be an error if the missing blocks are <= than the assumeutxo snapshot height, and the blo
...

💬 ryanofsky commented on pull request "index: make startup more efficient":
(https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1253746547)
In commit "make GetFirstStoredBlock assert that 'start_block' always has data" (aad83e489a9f515a1da36acd3e99f35b1a0da53c)

re: https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1248314125

> I think that the changes are minimal and help to reduce confusion. But let me know what you guys think.

Thanks for that. I think it would have been fine to just keep the `GetFirstStoredBlock` behavior unchanged, and just document it, but fixing the call sites is probably better in the long run
...

💬 S3RK commented on pull request "wallet: Give deprecation warning when loading a legacy wallet":
(https://github.com/bitcoin/bitcoin/pull/27869#issuecomment-1623103601)
reACK 8fbb6e99bfc85a1b9003cae402a7335843a86abd

💬 MarcoFalke commented on pull request "test: bugfix, synchronize indexes synchronously":
(https://github.com/bitcoin/bitcoin/pull/28026#issuecomment-1623118058)
> I mean, I'm also ok if we simply rise the timeout but, at the same time, I don't think that the background threads creation adds any meaningful test coverage here.

Right, it doesn't add any meaningful coverage. Though, I think the main point is that we should try to avoid adding test-only code to "real" code where possible. I am fine with anything, though.

🤔 S3RK reviewed a pull request: "Bump unconfirmed ancestor transactions to target feerate"
(https://github.com/bitcoin/bitcoin/pull/26152#pullrequestreview-1512188451)
Continue tests review

💬 S3RK commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1251575760)
nit: since you create new wallet for every test and never reuse any addresses do you really need to generate blocks? I tried to delete and the tests pass

💬 S3RK commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1251588782)
nit: here and other tests add `self.assert_spends_only_parent`

💬 S3RK commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1251589066)
nit: add `self.assert_spends_only_parent`

💬 S3RK commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1254037039)
nit: why there is a multiplier here?

💬 S3RK commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1251595920)
Here and other tests.I'm not sure the magic number is needed. I tried setting the multiplier to 1 and the tests still pass. Maybe just check that resulting fee rate is exactly equal to target fee rate?

💬 S3RK commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1254028212)
This could be extended to a set of parent txs to verify cases with multiple parents

💬 S3RK commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1254048054)
nit: sffo is not needed

💬 S3RK commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1254044504)
Isn't `test_preset_input_cpfp` testing this?

⚠️ megumin9 opened an issue: "The destructor of CCheckQueueControl may throw a exception "
(https://github.com/bitcoin/bitcoin/issues/28033)
In src/checkqueue.h, the destructor of CCheckQueueControl will call Wait, which may throw a condition_error exception in 'void condition_variable::wait(unique_lock<mutex>& m)'. This will cause a crash, Is it intended? If not, the exception from Wait can be caught as follows to avoid the crash.

if (!fDone)
Wait();
if (pqueue != nullptr) {
LEAVE_CRITICAL_SECTION(pqueue->ControlMutex);
}

--->

try {
if (!fDone)
Wait();
if (pqueue != nullptr) {
LEAVE_CRITICAL_SECTION(
...

💬 MarcoFalke commented on issue "The destructor of CCheckQueueControl may throw a exception ":
(https://github.com/bitcoin/bitcoin/issues/28033#issuecomment-1623201981)
What is `condition_error`?

💬 MarcoFalke commented on issue "The destructor of CCheckQueueControl may throw a exception ":
(https://github.com/bitcoin/bitcoin/issues/28033#issuecomment-1623240274)
Closing for now, but can be reopened when the outstanding questions have been answered.

✅ MarcoFalke closed an issue: "The destructor of CCheckQueueControl may throw a exception "
(https://github.com/bitcoin/bitcoin/issues/28033)

💬 MarcoFalke commented on issue "Unable to send funds":
(https://github.com/bitcoin/bitcoin/issues/28001#issuecomment-1623243536)
Closing for now. Leave a comment if this is still an issue.