💬 l0rinc commented on pull request "p2p: rename GetAddresses -> GetAddressesUncached":
(https://github.com/bitcoin/bitcoin/pull/32994#discussion_r2217888892)
I'm wondering if it makes more sense to signal the lack of something here (which is only relevant because another similar method exists) instead of the additional functionality in the pair - i.e. leave this and rename the other to `GetAddressesCached`, since that's the one that has an additional functionality, not this one.
(https://github.com/bitcoin/bitcoin/pull/32994#discussion_r2217888892)
I'm wondering if it makes more sense to signal the lack of something here (which is only relevant because another similar method exists) instead of the additional functionality in the pair - i.e. leave this and rename the other to `GetAddressesCached`, since that's the one that has an additional functionality, not this one.
💬 fanquake commented on pull request "log: [refactor] Use info level for init logs":
(https://github.com/bitcoin/bitcoin/pull/32967#issuecomment-3094654338)
> I personally would prefer doing every single LogPrintf -> LogInfo inlining here in a scripted diff
#29641.
(https://github.com/bitcoin/bitcoin/pull/32967#issuecomment-3094654338)
> I personally would prefer doing every single LogPrintf -> LogInfo inlining here in a scripted diff
#29641.
👋 l0rinc's pull request is ready for review: "test: revive test verifying that `GetCoinsCacheSizeState` switches from OK→LARGE→CRITICAL"
(https://github.com/bitcoin/bitcoin/pull/33021)
(https://github.com/bitcoin/bitcoin/pull/33021)
💬 l0rinc commented on pull request "log: [refactor] Use info level for init logs":
(https://github.com/bitcoin/bitcoin/pull/32967#issuecomment-3094666136)
> #29641
Yes, I'm aware - was there a discussion that I missed for why we're not doing that instead?
(https://github.com/bitcoin/bitcoin/pull/32967#issuecomment-3094666136)
> #29641
Yes, I'm aware - was there a discussion that I missed for why we're not doing that instead?
✅ achow101 closed an issue: "[SECURITY] Urgent Disclosure Coordination Request – High-Risk CI/CD Vulnerability"
(https://github.com/bitcoin/bitcoin/issues/33022)
(https://github.com/bitcoin/bitcoin/issues/33022)
💬 starixapp commented on issue "[SECURITY] Urgent Disclosure Coordination Request – High-Risk CI/CD Vulnerability":
(https://github.com/bitcoin/bitcoin/issues/33022#issuecomment-3094693429)
Let’s get something straight — both technically and ethically.
1. The issue reported was a **multi-stage CI/CD supply chain vulnerability**, not a syntax error or user complaint.
Dismissing it without technical review is a breach of every responsible disclosure principle in open source security.
2. The claim that “CI/CD cannot affect release builds” is not only inaccurate — it’s dangerously naive.
CI/CD affects everything *before* deterministic builds:
- Code review gates
-
...
(https://github.com/bitcoin/bitcoin/issues/33022#issuecomment-3094693429)
Let’s get something straight — both technically and ethically.
1. The issue reported was a **multi-stage CI/CD supply chain vulnerability**, not a syntax error or user complaint.
Dismissing it without technical review is a breach of every responsible disclosure principle in open source security.
2. The claim that “CI/CD cannot affect release builds” is not only inaccurate — it’s dangerously naive.
CI/CD affects everything *before* deterministic builds:
- Code review gates
-
...
⚠️ starixapp opened an issue: "Security Disclosure Mishandled by Bitcoin Core Maintainers"
(https://github.com/bitcoin/bitcoin/issues/33024)
Summary:
A serious coordinated disclosure of a CI/CD vulnerability chain impacting Bitcoin Core trust infrastructure was submitted responsibly, but met with the following:
- Dismissive behavior
- Public mockery of the researcher
- False technical claims downplaying CI/CD threat
- Closure of the GitHub issue without technical evaluation
#### Who was involved:
- **@achow101 (Ava Chow)** closed the issue without PoC review, discussion, or coordination.
- **@kanzure (Bryan Bishop)** mocked the dis
...
(https://github.com/bitcoin/bitcoin/issues/33024)
Summary:
A serious coordinated disclosure of a CI/CD vulnerability chain impacting Bitcoin Core trust infrastructure was submitted responsibly, but met with the following:
- Dismissive behavior
- Public mockery of the researcher
- False technical claims downplaying CI/CD threat
- Closure of the GitHub issue without technical evaluation
#### Who was involved:
- **@achow101 (Ava Chow)** closed the issue without PoC review, discussion, or coordination.
- **@kanzure (Bryan Bishop)** mocked the dis
...
✅ achow101 closed an issue: "Security Disclosure Mishandled by Bitcoin Core Maintainers"
(https://github.com/bitcoin/bitcoin/issues/33024)
(https://github.com/bitcoin/bitcoin/issues/33024)
💬 achow101 commented on issue "Security Disclosure Mishandled by Bitcoin Core Maintainers":
(https://github.com/bitcoin/bitcoin/issues/33024#issuecomment-3094700656)
A reply to your security list email was already sent, which you have also replied to. We have not received any concrete details.
Do not open duplicate issues.
(https://github.com/bitcoin/bitcoin/issues/33024#issuecomment-3094700656)
A reply to your security list email was already sent, which you have also replied to. We have not received any concrete details.
Do not open duplicate issues.
💬 starixapp commented on issue "Security Disclosure Mishandled by Bitcoin Core Maintainers":
(https://github.com/bitcoin/bitcoin/issues/33024#issuecomment-3094702578)
You’re demanding technical details — in public — without offering any secure coordination, and acting like that’s the standard?
Who exactly are you in this security process?
Because unless you’re the official point of contact for responsible disclosures (with actual authority to manage vulnerabilities), I have zero reason to disclose anything sensitive through a GitHub issue, especially under your command.
Security 101: You don't pressure researchers to publish exploit chains in public.
You
...
(https://github.com/bitcoin/bitcoin/issues/33024#issuecomment-3094702578)
You’re demanding technical details — in public — without offering any secure coordination, and acting like that’s the standard?
Who exactly are you in this security process?
Because unless you’re the official point of contact for responsible disclosures (with actual authority to manage vulnerabilities), I have zero reason to disclose anything sensitive through a GitHub issue, especially under your command.
Security 101: You don't pressure researchers to publish exploit chains in public.
You
...
💬 sipa commented on issue "Security Disclosure Mishandled by Bitcoin Core Maintainers":
(https://github.com/bitcoin/bitcoin/issues/33024#issuecomment-3094707151)
No, we are absolutely not asking you to disclose anything through github.
You received a response to your email to the security list to follow the instructions on https://bitcoincore.org/en/contact/ for responsible disclosure. It has PGP keys.
(https://github.com/bitcoin/bitcoin/issues/33024#issuecomment-3094707151)
No, we are absolutely not asking you to disclose anything through github.
You received a response to your email to the security list to follow the instructions on https://bitcoincore.org/en/contact/ for responsible disclosure. It has PGP keys.
🤔 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.