💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1721654337)
I like that my version just forwards everything in one statement, while your leaves 3, duplicated. But either one works for me.
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1721654337)
I like that my version just forwards everything in one statement, while your leaves 3, duplicated. But either one works for me.
💬 paplorinc commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1721677058)
In my suggestion there is not `std::span` involved and the size + data writing parts are split.
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1721677058)
In my suggestion there is not `std::span` involved and the size + data writing parts are split.
👍 danielabrozzoni approved a pull request: "test: check xor.dat creation when missing"
(https://github.com/bitcoin/bitcoin/pull/30669#pullrequestreview-2245365008)
tACK 07365d026127eb8435e7919a7793bd25d7f3ceca
(https://github.com/bitcoin/bitcoin/pull/30669#pullrequestreview-2245365008)
tACK 07365d026127eb8435e7919a7793bd25d7f3ceca
💬 paplorinc commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1721678712)
The above seemed to be working (that's why I suggested it originally) - but I'm not in love with this version either - thanks for considering.
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1721678712)
The above seemed to be working (that's why I suggested it originally) - but I'm not in love with this version either - thanks for considering.
💬 Sjors commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2296430206)
Concept ACK.
In my testing of #30664 I keep seeing `share/rpcauth/__pycache__/` in `git status`, so that might need to be added to `.gitignore`.
(https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2296430206)
Concept ACK.
In my testing of #30664 I keep seeing `share/rpcauth/__pycache__/` in `git status`, so that might need to be added to `.gitignore`.
🤔 paplorinc reviewed a pull request: "coins: remove logic for spent-and-FRESH cache entries and writing non-DIRTY entries"
(https://github.com/bitcoin/bitcoin/pull/30673#pullrequestreview-2245093564)
The code became a lot simpler now - have a few suggestion I'd like us to consider, but I understand if you think it's out of scope.
Once we get to a state that is ACK-worthy, I'll enable hard assertions everywhere and run an IBD and check if it crashes anywhere.
(https://github.com/bitcoin/bitcoin/pull/30673#pullrequestreview-2245093564)
The code became a lot simpler now - have a few suggestion I'd like us to consider, but I understand if you think it's out of scope.
Once we get to a state that is ACK-worthy, I'll enable hard assertions everywhere and run an IBD and check if it crashes anywhere.
💬 paplorinc 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_r1721733726)
```suggestion
Assume(IsDirty());
```
nit: it's a cheap call, why not make it an assert instead?
(https://github.com/bitcoin/bitcoin/pull/30673#discussion_r1721733726)
```suggestion
Assume(IsDirty());
```
nit: it's a cheap call, why not make it an assert instead?
💬 paplorinc 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_r1721727681)
I was surprise to see that the method called `GetCoin` now:
* overwrites the dummy coin parameter with the value found in the cache
* returns a boolean indicating whether the coin exists in the cache and is not spent (the name doesn't have any hints about it being spent or not)
I don't mind if you think it's outside the scope of the current change, but checking the usages it seems to me we can now simplify `GetCoin` to actually return the coin and let the callsite check for spent and it mig
...
(https://github.com/bitcoin/bitcoin/pull/30673#discussion_r1721727681)
I was surprise to see that the method called `GetCoin` now:
* overwrites the dummy coin parameter with the value found in the cache
* returns a boolean indicating whether the coin exists in the cache and is not spent (the name doesn't have any hints about it being spent or not)
I don't mind if you think it's outside the scope of the current change, but checking the usages it seems to me we can now simplify `GetCoin` to actually return the coin and let the callsite check for spent and it mig
...
💬 paplorinc 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_r1721668812)
So basically if we lay out the allowed combinations we'd get:
```C++
assert((!entry.IsDirty() & !entry.IsFresh() & !entry.coin.IsSpent()) | // attr = 0
(entry.IsDirty() & !entry.IsFresh() & !entry.coin.IsSpent()) | // attr = 1
(entry.IsDirty() & entry.IsFresh() & !entry.coin.IsSpent()) | // attr = 3
(entry.IsDirty() & !entry.IsFresh() & entry.coin.IsSpent())); // attr = 5
```
Which could be simplified to
```C++
assert((entry.IsDirty() && !(entry.coin.IsSpen
...
(https://github.com/bitcoin/bitcoin/pull/30673#discussion_r1721668812)
So basically if we lay out the allowed combinations we'd get:
```C++
assert((!entry.IsDirty() & !entry.IsFresh() & !entry.coin.IsSpent()) | // attr = 0
(entry.IsDirty() & !entry.IsFresh() & !entry.coin.IsSpent()) | // attr = 1
(entry.IsDirty() & entry.IsFresh() & !entry.coin.IsSpent()) | // attr = 3
(entry.IsDirty() & !entry.IsFresh() & entry.coin.IsSpent())); // attr = 5
```
Which could be simplified to
```C++
assert((entry.IsDirty() && !(entry.coin.IsSpen
...
💬 paplorinc 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_r1721738283)
is this is always the case, consider adding an assertion
```C++
assert(it->second.IsDirty());
```
(we can remove them in the last commit, but would be useful for the intermediary commits at least)
(https://github.com/bitcoin/bitcoin/pull/30673#discussion_r1721738283)
is this is always the case, consider adding an assertion
```C++
assert(it->second.IsDirty());
```
(we can remove them in the last commit, but would be useful for the intermediary commits at least)
💬 paplorinc 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_r1721510656)
when do we throw and when do we `Assume` here?
> \* Assume is the identity function.
> \*
> \* - Should be used to run non-fatal checks. In debug builds it behaves like
> \* Assert()/assert() to notify developers and testers about non-fatal errors.
> \* In production it doesn't warn or log anything.
> \* - For fatal errors, use Assert().
Wouldn't this be a fatal error?
Alternatively, if we want to separate the test runs from production runs, would it maybe make sense to call Sani
...
(https://github.com/bitcoin/bitcoin/pull/30673#discussion_r1721510656)
when do we throw and when do we `Assume` here?
> \* Assume is the identity function.
> \*
> \* - Should be used to run non-fatal checks. In debug builds it behaves like
> \* Assert()/assert() to notify developers and testers about non-fatal errors.
> \* In production it doesn't warn or log anything.
> \* - For fatal errors, use Assert().
Wouldn't this be a fatal error?
Alternatively, if we want to separate the test runs from production runs, would it maybe make sense to call Sani
...
💬 paplorinc 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_r1721740165)
same, can we add a (temporary) assertion for the removed value?
(https://github.com/bitcoin/bitcoin/pull/30673#discussion_r1721740165)
same, can we add a (temporary) assertion for the removed value?
👍 ryanofsky approved a pull request: "Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange and drop CRPCSignals"
(https://github.com/bitcoin/bitcoin/pull/30409#pullrequestreview-2245487748)
Code review ACK d880419cb65fd9d446057ea05384dad55da0f292. Only changes since list review are documentation improvements and a small code simplification.
Overall this PR makes nice improvements to the waitfor* RPCs and simplifies the code getting rid of boost signals and a global variable.
(https://github.com/bitcoin/bitcoin/pull/30409#pullrequestreview-2245487748)
Code review ACK d880419cb65fd9d446057ea05384dad55da0f292. Only changes since list review are documentation improvements and a small code simplification.
Overall this PR makes nice improvements to the waitfor* RPCs and simplifies the code getting rid of boost signals and a global variable.
💬 ryanofsky commented on pull request "Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange and drop CRPCSignals":
(https://github.com/bitcoin/bitcoin/pull/30409#discussion_r1721752519)
In commit "Replace RPCNotifyBlockChange with waitTipChanged()" (bf3377f4bf988f2477944ca54c4dee33b9b45b6e)
`now` variable is unused if timeout is zero, so this line could be moved inside the `if (timeout)` block below.
(https://github.com/bitcoin/bitcoin/pull/30409#discussion_r1721752519)
In commit "Replace RPCNotifyBlockChange with waitTipChanged()" (bf3377f4bf988f2477944ca54c4dee33b9b45b6e)
`now` variable is unused if timeout is zero, so this line could be moved inside the `if (timeout)` block below.
💬 instagibbs commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-2296543993)
@gmaxwell
>But, perhaps if there were padding traffic and a tiny amount of broadcast of third-party txn as if they were private broadcast then the weakness under the traffic analysis case would be improved to be no worse than you'd have from just running ordinary P2P across tor?
Assuming a switch to INV-based private relay, you might need to take some care to make sure the decoy tx gets requested at high rates, since the real private relay will tend to result in the getadata for the tx?
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-2296543993)
@gmaxwell
>But, perhaps if there were padding traffic and a tiny amount of broadcast of third-party txn as if they were private broadcast then the weakness under the traffic analysis case would be improved to be no worse than you'd have from just running ordinary P2P across tor?
Assuming a switch to INV-based private relay, you might need to take some care to make sure the decoy tx gets requested at high rates, since the real private relay will tend to result in the getadata for the tx?
💬 paplorinc commented on pull request "validation: write chainstate to disk every hour":
(https://github.com/bitcoin/bitcoin/pull/30611#issuecomment-2296577863)
I'll review this in more detail later, but I have a question first:
Whenever a production change is made without corresponding test changes (yet the build passes), it suggests to me that there might be a gap in our coverage.
Do you think it would make sense to add a test that reproduces some aspect of the old behavior before making the change—one that would need to be updated in the commit that introduces the change?
(https://github.com/bitcoin/bitcoin/pull/30611#issuecomment-2296577863)
I'll review this in more detail later, but I have a question first:
Whenever a production change is made without corresponding test changes (yet the build passes), it suggests to me that there might be a gap in our coverage.
Do you think it would make sense to add a test that reproduces some aspect of the old behavior before making the change—one that would need to be updated in the commit that introduces the change?
💬 andrewtoth commented on pull request "validation: write chainstate to disk every hour":
(https://github.com/bitcoin/bitcoin/pull/30611#issuecomment-2296587174)
> it would make sense to add a test that reproduces some aspect of the old behavior before making the change—one that would need to be updated in the commit that introduces the change?
Good point, I can add a test that that expects flush every 24 hours, then reduce it to 1 hour after this change.
(https://github.com/bitcoin/bitcoin/pull/30611#issuecomment-2296587174)
> it would make sense to add a test that reproduces some aspect of the old behavior before making the change—one that would need to be updated in the commit that introduces the change?
Good point, I can add a test that that expects flush every 24 hours, then reduce it to 1 hour after this change.
✅ furszy closed a pull request: "bugfix: update chainman best_header after block reconsideration"
(https://github.com/bitcoin/bitcoin/pull/29913)
(https://github.com/bitcoin/bitcoin/pull/29913)
💬 furszy commented on pull request "bugfix: update chainman best_header after block reconsideration":
(https://github.com/bitcoin/bitcoin/pull/29913#issuecomment-2296594476)
closing in favor of https://github.com/bitcoin/bitcoin/pull/30666.
(https://github.com/bitcoin/bitcoin/pull/29913#issuecomment-2296594476)
closing in favor of https://github.com/bitcoin/bitcoin/pull/30666.
💬 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_r1721800990)
This behavior is documented [here](https://github.com/bitcoin/bitcoin/blob/master/src/coins.h#L306-L310), so not sure why test code bothered to do this. Indeed, switching to `std::optional` would be cleaner, but I think a much bigger refactor. This is also suggested in the original PR [here](https://github.com/bitcoin/bitcoin/pull/18746#issuecomment-842393167).
(https://github.com/bitcoin/bitcoin/pull/30673#discussion_r1721800990)
This behavior is documented [here](https://github.com/bitcoin/bitcoin/blob/master/src/coins.h#L306-L310), so not sure why test code bothered to do this. Indeed, switching to `std::optional` would be cleaner, but I think a much bigger refactor. This is also suggested in the original PR [here](https://github.com/bitcoin/bitcoin/pull/18746#issuecomment-842393167).