🤔 Xekyo reviewed a pull request: "Bump unconfirmed ancestor transactions to target feerate"
(https://github.com/bitcoin/bitcoin/pull/26152#pullrequestreview-1511646387)
Addressed comments by @S3RK and @achow101

💬 Xekyo commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1251210983)
Used 3×target_fee_rate instead

💬 Xekyo commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1251243230)
I’m not sure I see a big difference between setting up the wallet with a confirmed UTXO from which I send an unconfirmed transaction with a varying feerate, or setting up the wallet to have an unconfirmed UTXO with varying feerate. The former may have the advantage that when I need multiple UTXOs, it’s easier to understand what’s different.

Currently expecting to keep it this way

💬 Xekyo commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1251248054)
Thanks, that check doesn’t make sense, I’ve removed it.

💬 Xekyo commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1251192852)
Thanks for catching this. Fixed.

💬 Xekyo commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1251203025)
Moved all comments about what the tests do to the implementations of the tests

💬 Xekyo commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1251204807)
Yeah, you’re right. Removed duplicate check.

💬 Xekyo commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1251230645)
Oops, I only fixed the rename in the next commit. Fixing.

🤔 mzumsande reviewed a pull request: "Rework validation logic for assumeutxo"
(https://github.com/bitcoin/bitcoin/pull/27746#pullrequestreview-1511762976)
Code Review ACK 03dfe04075668aa3b99a5d6d68adfcecffca0593

I reviewed the code again (didn't look that deep into the test changes), and did some manual testing.

> Not sure what's up with the arm failure, but tests pass for me locally.

Pretty sure it's unrelated, same thing just happened on master: https://github.com/bitcoin/bitcoin/runs/14733904119 (some discussion in #27988)

💬 mzumsande commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1251271660)
Now `ChainstateManager` has `ActiveSnapshotBase()` and `GetSnapshotBaseBlock()` which, as far as I understand it, do the same thing except for the caching.
Would it have been possible to add the caching to the existing `GetSnapshotBaseBlock()` instead?

💬 ajtowns commented on pull request "Allow configuring target block time for a signet":
(https://github.com/bitcoin/bitcoin/pull/27446#issuecomment-1619186257)
> @ajtowns I dont think we're gonna agree on this so whatever.

I don't understand why you're ignoring an approach that seems to do a better job of getting what you want (instant channels, without even having to wait a minute), and can work on mainnet (so won't end up developing a UX that's only conceivably good for demos and doesn't work with real money), and is already supported by CLN, LND and LDK. If you don't want to explain, or even just figure "if it's dumb but it works that's good eno
...

👍 ryanofsky approved a pull request: "Rework validation logic for assumeutxo"
(https://github.com/bitcoin/bitcoin/pull/27746#pullrequestreview-1505486978)
Partial code review ACK 03dfe04075668aa3b99a5d6d68adfcecffca0593. I've looked in detail at almost all of this except for tests. It seems like a big improvement and I haven't found any real issues at all. Am planning to review more soon.

💬 ryanofsky commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1246753697)
In commit "Move block-arrival information / preciousblock counters to ChainstateManager" (0dbc5805ca4371b7621bee0b7a553dac1a481614)

IIUC removing this line is not a change in behavior because the `nBlockSequenceId` variable was never used again after the `UnloadBlockIndex` call, outside of tests. Now there is new `ResetBlockSequenceCounters` function that tests can use to reset it separately. EDIT: Another review comment https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1243705025 als
...

💬 ryanofsky commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1246740699)
In commit "Move block-arrival information / preciousblock counters to ChainstateManager" (0dbc5805ca4371b7621bee0b7a553dac1a481614)

It would be nice to have a documentation comment for this function saying what it is for, especially since it is only called in a test. If the counter values are basically arbitrary and only used for ordering, I'm not sure why if they actually ever need to be reset, or if that's just a nice thing to do?

💬 ryanofsky commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1246815739)
In commit "test: Clear block index flags when testing snapshots" (e8168cd56c490609f59e1dc89add58b1be18b516)

This seems like a good change that makes test setup more realistic, but the could the commit message be updated to say what is motivating this change? Is it just for realism, or will a future change, or a certain kind of test depend on it?

💬 ryanofsky commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1246833752)
In commit "Move block-arrival information / preciousblock counters to ChainstateManager" (0dbc5805ca4371b7621bee0b7a553dac1a481614)

The test seems to pass without this line, at least in this commit.

If this is needed for a future change, would be good to say that in commit message. Or if this is just done for completeness would be good to say that in a code comment.

💬 ryanofsky commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1246822305)
In commit "test: Clear block index flags when testing snapshots" (e8168cd56c490609f59e1dc89add58b1be18b516)

Adding `BLOCK_ASSUMED_VALID` here seems surprising since I would expect `ActivateSnapshot` below to be setting `BLOCK_ASSUMED_VALID`, and comment above doesn't mention this flag. I think it would be good to drop this line or add a comment explaining why it is necessary.

💬 ryanofsky commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1246844224)
In commit "Move block-storage-related logic to ChainstateManager" (034f920d1755cff752d46b089ad37bdd703bf896)

Note: This change to the `CheckBlockIndex()` call is temporary and will revert back in later commit fb9c405506d6cfb6896e21b79789d7b6e638f6f2 "Move CheckBlockIndex() from Chainstate to ChainstateManager" when it becomes a `ChainstateManager` method.

There is not a change in behavior here in this commit, because in both places where
`Chainstate::AcceptBlock` was called previously (in
...

💬 ryanofsky commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1246903052)
In commit "Fix initialization of setBlockIndexCandidates when working with multiple chainstates" (c2bc5cfd9977838af334844675d77090fe9c3c74)

Can add a comment here explaining the logic? I guess the `pindex == GetSnapshotBaseBlock()` case needs to be a special case because the snapshot block may not have transactions?

Maybe also would be more efficient to call `ActiveSnapshotBase` instead of `GetSnapshotBaseBlock` here. I think I would move the commit introducing the `ActiveSnapshotBase` met
...

💬 ryanofsky commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1246842096)
In commit "Move block-storage-related logic to ChainstateManager" (034f920d1755cff752d46b089ad37bdd703bf896)

Notes: It seems logical to call `GetAll` for all chainstates and let the `TryAddBlockIndexCandidate` and `FindMostWorkChain` functions figure out how to use the block and which chain to attach it to.

There is a change in behavior here if there are multiple chainstates, because previously this would only add blocks to `setBlockIndexCandidates` for the active chain, and now it will ad
...