🤔 l0rinc reviewed a pull request: "coins: remove SetFresh method from CCoinsCacheEntry"
(https://github.com/bitcoin/bitcoin/pull/33018#pullrequestreview-3036128101)
Concept ACK
Very glad this is getting cleanup up, I found it worrying that a lot of tests were exercising invalid states in the first place - not the best foundation.
Quickly went over the changes, I find the small commits and excellent commit messages easy to follow.
I'll run a full IBD over this (maybe with some extra `SanityCheck` sprinkled around for extra measure) to make sure real usage doesn't trigger any invalid states we're not aware of.
(https://github.com/bitcoin/bitcoin/pull/33018#pullrequestreview-3036128101)
Concept ACK
Very glad this is getting cleanup up, I found it worrying that a lot of tests were exercising invalid states in the first place - not the best foundation.
Quickly went over the changes, I find the small commits and excellent commit messages easy to follow.
I'll run a full IBD over this (maybe with some extra `SanityCheck` sprinkled around for extra measure) to make sure real usage doesn't trigger any invalid states we're not aware of.
💬 l0rinc commented on pull request "coins: remove SetFresh method from CCoinsCacheEntry":
(https://github.com/bitcoin/bitcoin/pull/33018#discussion_r2217920312)
We should be able to remove `HaveCoin` now, since
```C++
bool CCoinsView::HaveCoin(const COutPoint &outpoint) const
{
return GetCoin(outpoint).has_value();
}
```
is more accurate (especially with the new extra assert)
(https://github.com/bitcoin/bitcoin/pull/33018#discussion_r2217920312)
We should be able to remove `HaveCoin` now, since
```C++
bool CCoinsView::HaveCoin(const COutPoint &outpoint) const
{
return GetCoin(outpoint).has_value();
}
```
is more accurate (especially with the new extra assert)
💬 l0rinc commented on pull request "coins: remove SetFresh method from CCoinsCacheEntry":
(https://github.com/bitcoin/bitcoin/pull/33018#discussion_r2217920517)
I understand if you think it may modify too many things here, but I'd prefer an actual `Assume` instead of the comment:
```suggestion
CoinsCachePair* Next() const noexcept
{
Assume(IsDirty());
return m_next;
}
```
(same for `Prev`)
(https://github.com/bitcoin/bitcoin/pull/33018#discussion_r2217920517)
I understand if you think it may modify too many things here, but I'd prefer an actual `Assume` instead of the comment:
```suggestion
CoinsCachePair* Next() const noexcept
{
Assume(IsDirty());
return m_next;
}
```
(same for `Prev`)
💬 l0rinc commented on pull request "coins: remove SetFresh method from CCoinsCacheEntry":
(https://github.com/bitcoin/bitcoin/pull/33018#discussion_r2217920379)
Now that we've removed it we should at least add an `Assume(!ret->second.coin.IsSpent());` here - or even an assert, since if we're not sure about this we shouldn't do it in the first place
(https://github.com/bitcoin/bitcoin/pull/33018#discussion_r2217920379)
Now that we've removed it we should at least add an `Assume(!ret->second.coin.IsSpent());` here - or even an assert, since if we're not sure about this we shouldn't do it in the first place
💬 l0rinc commented on pull request "coins: remove SetFresh method from CCoinsCacheEntry":
(https://github.com/bitcoin/bitcoin/pull/33018#discussion_r2217920827)
we might want to mention in the commit message why we can safely remove this now
(https://github.com/bitcoin/bitcoin/pull/33018#discussion_r2217920827)
we might want to mention in the commit message why we can safely remove this now
💬 l0rinc commented on pull request "coins: remove SetFresh method from CCoinsCacheEntry":
(https://github.com/bitcoin/bitcoin/pull/33018#discussion_r2217921211)
What should happen when `fresh` is `false`?
Shouldn't we assert something like:
```C++
if (fresh) {
pair.second.m_flags |= FRESH;
} else {
assert(!(pair.second.m_flags & FRESH));
}
(https://github.com/bitcoin/bitcoin/pull/33018#discussion_r2217921211)
What should happen when `fresh` is `false`?
Shouldn't we assert something like:
```C++
if (fresh) {
pair.second.m_flags |= FRESH;
} else {
assert(!(pair.second.m_flags & FRESH));
}
💬 l0rinc commented on pull request "coins: remove SetFresh method from CCoinsCacheEntry":
(https://github.com/bitcoin/bitcoin/pull/33018#discussion_r2217922071)
definition of `fresh` should likely include `dirty` now:
```C++
const auto fresh{dirty && fuzzed_data_provider.ConsumeBool()};
```
(https://github.com/bitcoin/bitcoin/pull/33018#discussion_r2217922071)
definition of `fresh` should likely include `dirty` now:
```C++
const auto fresh{dirty && fuzzed_data_provider.ConsumeBool()};
```
💬 l0rinc commented on pull request "coins: remove SetFresh method from CCoinsCacheEntry":
(https://github.com/bitcoin/bitcoin/pull/33018#discussion_r2217923008)
nit: for consistency with other overrides:
```suggestion
if (auto it{m_data.find(outpoint)}; it != m_data.end()) {
```
(https://github.com/bitcoin/bitcoin/pull/33018#discussion_r2217923008)
nit: for consistency with other overrides:
```suggestion
if (auto it{m_data.find(outpoint)}; it != m_data.end()) {
```
💬 l0rinc commented on pull request "coins: remove SetFresh method from CCoinsCacheEntry":
(https://github.com/bitcoin/bitcoin/pull/33018#discussion_r2217921830)
nit: for consistency with other `SetDirty` calls we could swap the parameter order:
```suggestion
BOOST_CHECK(n2.second.IsDirty() && n2.second.IsFresh());
```
----
Alternatively we could rename `IsFresh` to `IsDirtyAndFresh` and assert that if fresh is true, so is dirty - which would simplify this line to asserting just a single method.
(https://github.com/bitcoin/bitcoin/pull/33018#discussion_r2217921830)
nit: for consistency with other `SetDirty` calls we could swap the parameter order:
```suggestion
BOOST_CHECK(n2.second.IsDirty() && n2.second.IsFresh());
```
----
Alternatively we could rename `IsFresh` to `IsDirtyAndFresh` and assert that if fresh is true, so is dirty - which would simplify this line to asserting just a single method.
💬 l0rinc commented on pull request "coins: remove SetFresh method from CCoinsCacheEntry":
(https://github.com/bitcoin/bitcoin/pull/33018#discussion_r2217925887)
great, now that we have this, can we call `SanityCheck` in more tests? I understood that wasn't the case since we still had invalid states in fuzz/unit tests
(https://github.com/bitcoin/bitcoin/pull/33018#discussion_r2217925887)
great, now that we have this, can we call `SanityCheck` in more tests? I understood that wasn't the case since we still had invalid states in fuzz/unit tests
💬 starixapp commented on issue "Security Disclosure Mishandled by Bitcoin Core Maintainers":
(https://github.com/bitcoin/bitcoin/issues/33024#issuecomment-3094723332)
Subject: Escalation – Zero Response from Security Team
Hi,
I’ve already sent my initial report via email to security@bitcoincore.org — as instructed.
That was overhours ago. Not a single person from your team acknowledged it or treated it with any seriousness.
Instead of coordinated handling, I’m receiving public deflection, demands to encrypt before engagement, and complete disregard for the time-critical nature of this vulnerability.
If someone *actually* capable of understanding what’s
...
(https://github.com/bitcoin/bitcoin/issues/33024#issuecomment-3094723332)
Subject: Escalation – Zero Response from Security Team
Hi,
I’ve already sent my initial report via email to security@bitcoincore.org — as instructed.
That was overhours ago. Not a single person from your team acknowledged it or treated it with any seriousness.
Instead of coordinated handling, I’m receiving public deflection, demands to encrypt before engagement, and complete disregard for the time-critical nature of this vulnerability.
If someone *actually* capable of understanding what’s
...
💬 starixapp commented on issue "Security Disclosure Mishandled by Bitcoin Core Maintainers":
(https://github.com/bitcoin/bitcoin/issues/33024#issuecomment-3094728580)
Final Warning – This Is Not a Game
Hi,
The level of negligence and dismissal I’ve encountered from this process is
honestly unbelievable.
Let me make something very clear:
You are sitting on a live vulnerability chain capable of compromising trust
in Bitcoin Core CI/CD, poisoning builds, and potentially diverting funds
from real users — *without detection*.
This isn’t a UI bug. This is a structural flaw. And you’re treating it like
an inconvenience.
By continuing to downplay
...
(https://github.com/bitcoin/bitcoin/issues/33024#issuecomment-3094728580)
Final Warning – This Is Not a Game
Hi,
The level of negligence and dismissal I’ve encountered from this process is
honestly unbelievable.
Let me make something very clear:
You are sitting on a live vulnerability chain capable of compromising trust
in Bitcoin Core CI/CD, poisoning builds, and potentially diverting funds
from real users — *without detection*.
This isn’t a UI bug. This is a structural flaw. And you’re treating it like
an inconvenience.
By continuing to downplay
...
💬 starixapp commented on issue "Security Disclosure Mishandled by Bitcoin Core Maintainers":
(https://github.com/bitcoin/bitcoin/issues/33024#issuecomment-3094731049)
You're Not Protecting Bitcoin – You're Endangering It
Hi,
Let me be blunt:
If this level of dismissiveness continues, it won’t be the vulnerability that destroys trust in Bitcoin — it’ll be your attitude toward it.
Bitcoin Core is older than some of the responses I’ve been getting.
This isn’t just software. It’s a multi-billion dollar system that millions rely on.
And yet here we are, wasting time with gatekeeping, arrogance, and red tape.
You don’t protect Bitcoin by mocking researcher
...
(https://github.com/bitcoin/bitcoin/issues/33024#issuecomment-3094731049)
You're Not Protecting Bitcoin – You're Endangering It
Hi,
Let me be blunt:
If this level of dismissiveness continues, it won’t be the vulnerability that destroys trust in Bitcoin — it’ll be your attitude toward it.
Bitcoin Core is older than some of the responses I’ve been getting.
This isn’t just software. It’s a multi-billion dollar system that millions rely on.
And yet here we are, wasting time with gatekeeping, arrogance, and red tape.
You don’t protect Bitcoin by mocking researcher
...
💬 starixapp commented on issue "Security Disclosure Mishandled by Bitcoin Core Maintainers":
(https://github.com/bitcoin/bitcoin/issues/33024#issuecomment-3094732102)
@laanwj
I’m respectfully tagging you because this has gone beyond a misunderstanding — and into a systemic failure of responsible disclosure.
What started as a coordinated attempt to report a critical CI/CD vulnerability has now turned into a thread filled with mockery, deflection, and a complete lack of technical engagement from some contributors.
Bitcoin Core deserves better than “lol encrypt first” replies and GitHub closures.
This isn’t about me — it’s about how you treat infrastructur
...
(https://github.com/bitcoin/bitcoin/issues/33024#issuecomment-3094732102)
@laanwj
I’m respectfully tagging you because this has gone beyond a misunderstanding — and into a systemic failure of responsible disclosure.
What started as a coordinated attempt to report a critical CI/CD vulnerability has now turned into a thread filled with mockery, deflection, and a complete lack of technical engagement from some contributors.
Bitcoin Core deserves better than “lol encrypt first” replies and GitHub closures.
This isn’t about me — it’s about how you treat infrastructur
...
📝 marshallshen opened a pull request: "test: add unit test for psbt_wallet error messages"
(https://github.com/bitcoin/bitcoin/pull/33025)
# Summary
This PR adds a new unit test `test_error_messages` to verify that all `PSBTError` enum values return the correct error messages from the `PSBTErrorString()` function in `messages.cpp`.
## Motivation
- Ensures complete test coverage of the `PSBTErrorString()` function in `src/common/messages.cpp`
- Provides regression protection against accidental changes to user-facing error messages
- Documents the expected error messages for all PSBT error cases
## Changes
- **Added:** `
...
(https://github.com/bitcoin/bitcoin/pull/33025)
# Summary
This PR adds a new unit test `test_error_messages` to verify that all `PSBTError` enum values return the correct error messages from the `PSBTErrorString()` function in `messages.cpp`.
## Motivation
- Ensures complete test coverage of the `PSBTErrorString()` function in `src/common/messages.cpp`
- Provides regression protection against accidental changes to user-facing error messages
- Documents the expected error messages for all PSBT error cases
## Changes
- **Added:** `
...
💬 starixapp commented on issue "Security Disclosure Mishandled by Bitcoin Core Maintainers":
(https://github.com/bitcoin/bitcoin/issues/33024#issuecomment-3094736694)
Final Notice – Mishandled Disclosure with Global Financial Impact
To whom it may still concern,
This is my final attempt to engage professionally.
What I disclosed is not a bug. It is not a user-side issue.
It is a **full-spectrum CI/CD trust chain vulnerability** that affects the integrity of Bitcoin Core's build process, its signed binaries, and the downstream systems that trust them — including wallets, exchanges, infrastructure providers, and even governments.
The current response from
...
(https://github.com/bitcoin/bitcoin/issues/33024#issuecomment-3094736694)
Final Notice – Mishandled Disclosure with Global Financial Impact
To whom it may still concern,
This is my final attempt to engage professionally.
What I disclosed is not a bug. It is not a user-side issue.
It is a **full-spectrum CI/CD trust chain vulnerability** that affects the integrity of Bitcoin Core's build process, its signed binaries, and the downstream systems that trust them — including wallets, exchanges, infrastructure providers, and even governments.
The current response from
...
💬 marshallshen commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r2217937651)
Follow up: I created a PR to add test coverage: https://github.com/bitcoin/bitcoin/pull/33025
Will ensure it passes all the CI checks, 🙇
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r2217937651)
Follow up: I created a PR to add test coverage: https://github.com/bitcoin/bitcoin/pull/33025
Will ensure it passes all the CI checks, 🙇
💬 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!