💬 fjahr commented on pull request "test: Improve clarity of subsidy limit test":
(https://github.com/bitcoin/bitcoin/pull/30699#discussion_r1727686414)
And why remove this?

👍 hodlinator approved a pull request: "node: reduce unsafe uint256S usage"
(https://github.com/bitcoin/bitcoin/pull/30569#pullrequestreview-2255475669)
re-ACK 81554aac80bf2270db977c110c37acc7e8034194

`git range-diff master cf88a55e97719aabd62f0b608df0800fef8304de 81554aac80bf2270db977c110c37acc7e8034194`

Only changes `BOOST_AUTO_TEST_CASE(from_user_hex)` tests, error strings, commit messages and revives the `LogPrintf` -> `LogInfo` update.

💬 hodlinator commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1727696360)
nit: Inadvertent newline removal?

💬 hodlinator commented on pull request "Add a "tx output spender" index":
(https://github.com/bitcoin/bitcoin/pull/24539#discussion_r1727703220)
Better than my suggestion, thanks!

💬 l0rinc commented on pull request "test: Improve clarity of subsidy limit test":
(https://github.com/bitcoin/bitcoin/pull/30699#discussion_r1727707662)
> I do not like that the test no longer checks against max subsidy size and overflow/unreasonableness of the sum. What is your reasoning around that?

Thanks for the review! I aimed to view the code from the perspective of layman Bitcoin enthusiasts, trying to get an answer to their question, "How do we know the limit isn't greater than 21 million?".
I prioritized a high signal-to-noise ratio, omitting checks already covered by other tests. This test was about validating limits, not `GetBlock
...

💬 l0rinc commented on pull request "test: Improve clarity of subsidy limit test":
(https://github.com/bitcoin/bitcoin/pull/30699#discussion_r1727707982)
Please see: https://github.com/bitcoin/bitcoin/pull/30699#discussion_r1727707662

💬 l0rinc commented on pull request "test: Improve clarity of subsidy limit test":
(https://github.com/bitcoin/bitcoin/pull/30699#issuecomment-2305497107)
> nSubsidyHalvingInterval being evenly divisible by 1000

That shouldn't be important in the new implementation anymore since we're counting every block until the block reward exists.

> The old test went on for ~2x the subsidy-granting height, seems like a limit as least as good as 10m?

Reverted to 14m, thanks.

💬 fjahr commented on pull request "test: Improve clarity of subsidy limit test":
(https://github.com/bitcoin/bitcoin/pull/30699#discussion_r1727720687)
> I aimed to view the code from the perspective of layman Bitcoin enthusiasts

I don't think that many of them will look at our unit tests and if they do it's probably easier if you stick to writing a comment that is understandable. But I don't think they are the target audience of our unit tests.

> I prioritized a high signal-to-noise ratio

I don't think that's the right strategy for our unit tests in general. I don't think we should remove checks unless there is a clear reason to do so
...

💬 fjahr commented on pull request "test: Improve clarity of subsidy limit test":
(https://github.com/bitcoin/bitcoin/pull/30699#issuecomment-2305510069)
As I indicated above, even with the changes, approach NACK from me. I think clarification can be achieved without a complete refactoring of this test. I'm neutral on adding comments and more checks.

💬 l0rinc commented on pull request "test: Improve clarity of subsidy limit test":
(https://github.com/bitcoin/bitcoin/pull/30699#discussion_r1727725376)
I've added back the intermediary asserts.

👋 ismaelsadeeq's pull request is ready for review: "Wallet, Bugfix: Lock Wallet Context mutex Before Adding/Removing Settings"
(https://github.com/bitcoin/bitcoin/pull/30697)

💬 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_r1727754817)
@l0rinc It seems to me like you are thinking this PR has any behavior change. Any coin retrieved from `GetCoin` will be unspent if `GetCoin` returns true. You can follow this logically from seeing the return value [here](https://github.com/bitcoin/bitcoin/blob/master/src/coins.cpp#L65) and then the only base class is `CCoinsViewDB` which does not save spent coins so cannot retrieve them in `GetCoin`.

This PR is recognizing that `FetchCoin` logically cannot get to [here](https://github.com/bit
...

💬 ismaelsadeeq commented on pull request "Wallet, Bugfix: Lock Wallet Context mutex Before Adding/Removing Settings":
(https://github.com/bitcoin/bitcoin/pull/30697#discussion_r1727755299)
done

👍 tdb3 approved a pull request: "test: replace deprecated secp256k1 context flags usage"
(https://github.com/bitcoin/bitcoin/pull/30687#pullrequestreview-2255570973)
light CR and test ACK 60055f1abc4b4ad5f66a2fcf2e61c65efc777036

Thanks for finding/fixing.
Ran unit tests locally (including `key_tests`) (passed).
grep'ed `src/` for other (non `src/secp256k1/*`) instances of `SECP256K1_CONTEXT_SIGN` but didn't see any.

💬 ismaelsadeeq commented on pull request "Wallet, Bugfix: Lock Wallet Context mutex Before Adding/Removing Settings":
(https://github.com/bitcoin/bitcoin/pull/30697#discussion_r1727759303)
Done thanks.

💬 ismaelsadeeq commented on pull request "Wallet, Bugfix: Lock Wallet Context mutex Before Adding/Removing Settings":
(https://github.com/bitcoin/bitcoin/pull/30697#discussion_r1727762827)
Done.

💬 ismaelsadeeq commented on pull request "Wallet, Bugfix: Lock Wallet Context mutex Before Adding/Removing Settings":
(https://github.com/bitcoin/bitcoin/pull/30697#discussion_r1727763363)
Added

💬 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_r1727769365)
Yeah, I understand that.

What I'm struggling with is the cache retrieval, and especially the invalidation logic, which seems to be scattered throughout the code and tightly coupled with the business logic.

Would you be open to a screen-sharing session to discuss this? It would help me better understand the constraints and likely speed up my review process as well.

💬 ismaelsadeeq commented on pull request "Wallet, Bugfix: Lock Wallet Context mutex Before Adding/Removing Settings":
(https://github.com/bitcoin/bitcoin/pull/30697#discussion_r1727770914)
Done.

💬 ismaelsadeeq commented on pull request "Wallet, Bugfix: Lock Wallet Context mutex Before Adding/Removing Settings":
(https://github.com/bitcoin/bitcoin/pull/30697#discussion_r1727771800)

This is what was written initially.
Clang format changed it to this format, which I thought was the recommended approach:
https://github.com/bitcoin/bitcoin/tree/master/contrib/devtools#clang-format-diffpy

I've now switched to your suggestion.