✅ willcl-ark closed an issue: "Atomic "
(https://github.com/bitcoin/bitcoin/issues/30848)

💬 l0rinc commented on pull request "validation: write chainstate to disk every hour":
(https://github.com/bitcoin/bitcoin/pull/30611#discussion_r1749324715)
you can resolve this comment

💬 l0rinc commented on pull request "Drop -dbcache limit":
(https://github.com/bitcoin/bitcoin/pull/28358#issuecomment-2336790306)
Concept ACK, I'll run some tests later

💬 ismaelsadeeq commented on pull request "interfaces: #30697 follow ups":
(https://github.com/bitcoin/bitcoin/pull/30828#discussion_r1749332974)
There are two suggestions here:

1. **@furszy suggested to fail when the value is null**: I agree with not checking for null values, for the reasons outlined above.

2. **@ryanofsky suggested removing the null value check**: This approach would overwrite the previous settings with a null value.

> Setting a null value should consistently delete the setting, rather than having different behavior in different functions.

However, if we don’t delete the key-value pair, the setting sti
...

💬 ismaelsadeeq commented on pull request "interfaces: #30697 follow ups":
(https://github.com/bitcoin/bitcoin/pull/30828#discussion_r1749339659)
I guess you are trying to avoid a situation where we would need to write:

```
return action == interfaces::SettingsAction::SKIP_WRITE || action == interfaces::SettingsAction::NEW_ACTION || args().WriteSettingsFile();
```

In some cases we would still need to modify this to handle the new action if it has additional effects beyond just writing, such as writing and logging, for example.

---
I've updated to your suggestion, as it is more maintainable.

💬 sipsorcery commented on pull request "build: Minimize I/O operations in `GenerateHeaderFrom{Json,Raw}.cmake`":
(https://github.com/bitcoin/bitcoin/pull/30842#issuecomment-2336808242)
> @sipsorcery What do you think?

The overall build time for me on Win11 msvc reduced by 44s (only one test run each of the master and PR builds).

**master**:
```
PS C:\Dev\github\bitcoin> Measure-Command { cmake --build build --config Release }
TotalSeconds : 637.2643505
```

**pr 30842**:
```
PS C:\Dev\github\bitcoin> Measure-Command { cmake --build build --config Release }
TotalSeconds : 553.3287435
```

IMHO it's always beneficial to improve CI times so +1 for me
...

💬 l0rinc commented on pull request "coins: remove logic for spent-and-FRESH cache entries and writing non-DIRTY entries":
(https://github.com/bitcoin/bitcoin/pull/30673#discussion_r1749350258)
Did this change require any other test adjustment?

💬 sipsorcery commented on pull request "build: Minor build system fixes and amendments":
(https://github.com/bitcoin/bitcoin/pull/30803#issuecomment-2336816423)
tACK 1cc93fe7b40f10a7d1d1189058af98a2bce31381 (win11 msvc).

📝 l0rinc opened a pull request: "refactor: Remove unrealistic simulation state"
(https://github.com/bitcoin/bitcoin/pull/30849)
While reviewing the PR regarding [the removal of impossible Coin cache logic](https://github.com/bitcoin/bitcoin/pull/30673/files#r1740154464), we've noticed that the related tests often [reflect impossible states](https://github.com/bitcoin/bitcoin/pull/30673/files#r1740154464).

Some of these states are possible [because of the design](https://github.com/bitcoin/bitcoin/pull/30673/files#r1748087322) of the `GetCoin` method.
Browsing the Coin cache refactoring history revealed that migrating
...

💬 andrewtoth commented on pull request "coins: remove logic for spent-and-FRESH cache entries and writing non-DIRTY entries":
(https://github.com/bitcoin/bitcoin/pull/30673#discussion_r1749352045)
No, but without this the next commit would break.

💬 l0rinc commented on pull request "coins: remove logic for spent-and-FRESH cache entries and writing non-DIRTY entries":
(https://github.com/bitcoin/bitcoin/pull/30673#discussion_r1749355280)
Added https://github.com/bitcoin/bitcoin/pull/30849 to somewhat mitigate the `GetCoin` mutations in tests - hoping this will get us closer to excluding invalid tests states.

💬 andrewtoth commented on pull request "refactor: migrate `bool GetCoin` to return `optional<Coin>`":
(https://github.com/bitcoin/bitcoin/pull/30849#discussion_r1749356667)
nit: don't remove space before `:`.

🤔 andrewtoth reviewed a pull request: "refactor: migrate `bool GetCoin` to return `optional<Coin>`"
(https://github.com/bitcoin/bitcoin/pull/30849#pullrequestreview-2288602488)
Concept ACK

💬 andrewtoth commented on pull request "refactor: migrate `bool GetCoin` to return `optional<Coin>`":
(https://github.com/bitcoin/bitcoin/pull/30849#discussion_r1749357863)
For clarity this comment should go above where we use `m_rng`. So it should go a few lines up and probably be changed to `Randomly return spent coins`.

💬 andrewtoth commented on pull request "refactor: migrate `bool GetCoin` to return `optional<Coin>`":
(https://github.com/bitcoin/bitcoin/pull/30849#discussion_r1749358772)
In commit 6243ceb65b83ea8dc3a689fdcc868a3a94b86e80 ` refactor: Remove unrealistic simulation state `

The comment above should be removed in this commit as well, since it is no longer accurate.

💬 l0rinc commented on pull request "refactor: migrate `bool GetCoin` to return `optional<Coin>`":
(https://github.com/bitcoin/bitcoin/pull/30849#discussion_r1749359238)
I'll just remove it

📝 klein818 opened a pull request: "doc: fix minor typo "
(https://github.com/bitcoin/bitcoin/pull/30850)
Fix typo in doc/build-windows-msvc.md:
- "Micsrosoft" -> Microsoft

No test required.

💬 l0rinc commented on pull request "refactor: migrate `bool GetCoin` to return `optional<Coin>`":
(https://github.com/bitcoin/bitcoin/pull/30849#discussion_r1749359555)
Already done, forgot to push

💬 l0rinc commented on pull request "refactor: migrate `bool GetCoin` to return `optional<Coin>`":
(https://github.com/bitcoin/bitcoin/pull/30849#discussion_r1749359570)
Done

💬 l0rinc commented on pull request "doc: fix minor typo":
(https://github.com/bitcoin/bitcoin/pull/30850#issuecomment-2336825778)
ACK 7a669fde183b2f98c4faeff3eedaf95a1cba3b09