💬 fjahr commented on pull request "test: add unit test for psbt_wallet error messages":
(https://github.com/bitcoin/bitcoin/pull/33025#issuecomment-3094741352)
Unfortunately this 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?
Direct coverage of the `PSBTErrorString` function isn't very valuable IMO because it is just a wrapper of a switch statement which can be tested implicitly with tests such as I mention above. Test that are this closely tied to implementations are usually more hassle than they are wort
...
(https://github.com/bitcoin/bitcoin/pull/33025#issuecomment-3094741352)
Unfortunately this 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?
Direct coverage of the `PSBTErrorString` function isn't very valuable IMO because it is just a wrapper of a switch statement which can be tested implicitly with tests such as I mention above. Test that are this closely tied to implementations are usually more hassle than they are wort
...
💬 achow101 commented on issue "Security Disclosure Mishandled by Bitcoin Core Maintainers":
(https://github.com/bitcoin/bitcoin/issues/33024#issuecomment-3094745793)
You have been asked multiple times to send an encrypted email to the security list. No one has asked you to publicly disclose any issues.
Your repeated claims and accusations of had faith strain your credibility.
(https://github.com/bitcoin/bitcoin/issues/33024#issuecomment-3094745793)
You have been asked multiple times to send an encrypted email to the security list. No one has asked you to publicly disclose any issues.
Your repeated claims and accusations of had faith strain your credibility.
💬 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!
(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
(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.
(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
...
(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.
(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?
(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.
(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.
(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.
(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
(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`?
(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.
(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
(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.
(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.
(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.
(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.
(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 👍
(https://github.com/bitcoin/bitcoin/pull/33018#discussion_r2218069073)
You did enable it for a few additional cases, that's already a win 👍