💬 marshallshen commented on pull request "test: add unit test for psbt_wallet error messages":
(https://github.com/bitcoin/bitcoin/pull/33025#issuecomment-3094746159)
> his is not what I had in mind when I wrote my comment. What I meant was that there should be a test that triggers the uncovered error message through wallet usage. Can you give this a try instead?

Gotcha, so it's more of a functional test for the behavior that triggers `PSBTError::INCOMPLETE`,
Not a unit test to cover `PSBTErrorString`.

And I agree that testing case statement isn't really super valuable. I will give it another try, thanks!

💬 pinheadmz commented on issue "Security Disclosure Mishandled by Bitcoin Core Maintainers":
(https://github.com/bitcoin/bitcoin/issues/33024#issuecomment-3094754518)
Blocking this user for 7 days for misbehavior on the issue tracker. Seems like appropriate channels are open for proper security disclosure

🤔 Prabhat1308 reviewed a pull request: "test: Fix reorg patterns in tests to use proper fork-based approach"
(https://github.com/bitcoin/bitcoin/pull/32587#pullrequestreview-3036175447)
ACK [`cec7ce0`](https://github.com/bitcoin/bitcoin/pull/32587/commits/cec7ce0af811f6389211b1e5b97c84c3ffb9444c)

changes since I last reviewed
- the PR has been extended to 5 more tests other than `mempool_reorg.py`
- Tested and reviewed the added tests.

📝 fjahr opened a pull request: "test, refactor: Embedded ASmap selected preparatory work"
(https://github.com/bitcoin/bitcoin/pull/33026)
This contains four commits from #28792 that can be easily reviewed and merged independently. I hope splitting this change off can make this part move a bit faster and reduce frequency of needed rebases for #28792.

The commits in order:
- Add additional unit test vectors to the asmap interpreter (written by sipa). This helps to ensure that the further refactors in #28792 don't change behavior.
- Modernizes the logging in `util/asmap.cpp`, I added this while touching the rest of the file all
...

💬 fjahr commented on pull request "Embed default ASMap as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#issuecomment-3094816503)
Rebased and I shaved off 4 easy to review commits from here and put them into #33026. Maybe splitting this off helps to move things along a bit faster.

💬 l0rinc commented on pull request "coins: remove SetFresh method from CCoinsCacheEntry":
(https://github.com/bitcoin/bitcoin/pull/33018#discussion_r2218040983)
note: the cursor contains dirty entries now, so `if (!it->second.IsDirty()) {` on line 186 is also a noop - could we add a TODO there to signal that we're aware of it but want to tackle it in a follow-up?

💬 andrewtoth commented on pull request "coins: remove SetFresh method from CCoinsCacheEntry":
(https://github.com/bitcoin/bitcoin/pull/33018#discussion_r2218050169)
Done.

💬 andrewtoth commented on pull request "coins: remove SetFresh method from CCoinsCacheEntry":
(https://github.com/bitcoin/bitcoin/pull/33018#discussion_r2218050233)
Done.

💬 andrewtoth commented on pull request "coins: remove SetFresh method from CCoinsCacheEntry":
(https://github.com/bitcoin/bitcoin/pull/33018#discussion_r2218050997)
I don't see how that is different. `m_flags` being set implies `IsDirty()` now, so it is equivalent. I prefer not to change this.

💬 l0rinc commented on pull request "coins: remove SetFresh method from CCoinsCacheEntry":
(https://github.com/bitcoin/bitcoin/pull/33018#discussion_r2218053411)
`Assume(m_flags);` would be true for fresh only - `Assume(IsDirty());` is stricter

💬 l0rinc commented on pull request "coins: remove SetFresh method from CCoinsCacheEntry":
(https://github.com/bitcoin/bitcoin/pull/33018#discussion_r2218054579)
Isn't that the same as the parent in `CCoinsView`?

💬 andrewtoth commented on pull request "coins: remove SetFresh method from CCoinsCacheEntry":
(https://github.com/bitcoin/bitcoin/pull/33018#discussion_r2218061776)
It's not possible to set fresh only now.

💬 l0rinc commented on pull request "coins: remove SetFresh method from CCoinsCacheEntry":
(https://github.com/bitcoin/bitcoin/pull/33018#discussion_r2218064237)
Exactly, hence my recommendation

💬 andrewtoth commented on pull request "coins: remove SetFresh method from CCoinsCacheEntry":
(https://github.com/bitcoin/bitcoin/pull/33018#discussion_r2218067867)
Done.

💬 andrewtoth commented on pull request "coins: remove SetFresh method from CCoinsCacheEntry":
(https://github.com/bitcoin/bitcoin/pull/33018#discussion_r2218068273)
We can't since we are still calling this with spent, dirty, and fresh entries. These would be deleted in production code. That can also be tackled in a follow-up.

💬 andrewtoth commented on pull request "coins: remove SetFresh method from CCoinsCacheEntry":
(https://github.com/bitcoin/bitcoin/pull/33018#discussion_r2218068575)
There are quite a few places where we can clean up production and test logic to not have to deal with non-dirty entries in BatchWrite. I think there would be too many TODOs.

💬 andrewtoth commented on pull request "coins: remove SetFresh method from CCoinsCacheEntry":
(https://github.com/bitcoin/bitcoin/pull/33018#discussion_r2218068664)
Done.

💬 l0rinc commented on pull request "coins: remove SetFresh method from CCoinsCacheEntry":
(https://github.com/bitcoin/bitcoin/pull/33018#discussion_r2218069073)
You did enable it for a few additional cases, that's already a win 👍

💬 andrewtoth commented on pull request "coins: remove SetFresh method from CCoinsCacheEntry":
(https://github.com/bitcoin/bitcoin/pull/33018#discussion_r2218069128)
Done.

💬 andrewtoth commented on pull request "coins: remove SetFresh method from CCoinsCacheEntry":
(https://github.com/bitcoin/bitcoin/pull/33018#discussion_r2218070113)
Err, I did but didn't test locally before pushing. I will have to revert. Tests are failing.