CoinsRegistry: Refactor DescendantOf
#11697
Conversation
kiminuo
force-pushed
the
feature/2023-10-14-Simplify-ICoinsView
branch
from
fa0c139
to
4feada5
Compare
| @@ -239,7 +240,7 @@ public void SwitchToUnconfirmFromBlock(Height blockHeight) | |||
| { | |||
| foreach (var coin in AsCoinsViewNoLock().AtBlockHeight(blockHeight)) | |||
| { | |||
| var descendantCoins = DescendantOfAndSelf(coin); | |||
There was a problem hiding this comment.
NoLock variant should be used here.
| @@ -11,16 +11,10 @@ public interface ICoinsView : IEnumerable<SmartCoin> | |||
|
|
|||
| ICoinsView Available(); | |||
|
|
|||
| ICoinsView ChildrenOf(SmartCoin coin); | |||
There was a problem hiding this comment.
Only needed for computing of descendants.
| ? EmptyCoinsView | ||
| : new CoinsView(Coins.Where(x => x.TransactionId == coin.SpenderTransaction.GetHash())); | ||
|
|
||
| public ICoinsView DescendantOf(SmartCoin coin) |
There was a problem hiding this comment.
Moved to CoinsRegistry
kiminuo
force-pushed
the
feature/2023-10-14-Simplify-ICoinsView
branch
from
7c9741b
to
f0c9669
Compare
| private ImmutableArray<SmartCoin> DescendantOfAndSelfNoLock(SmartCoin coin) | ||
| { | ||
| HashSet<SmartCoin> allCoins = new(Coins); | ||
| allCoins.UnionWith(SpentCoins); |
There was a problem hiding this comment.
AsAllCoinsViewNoLock() should be used here, I believe.
There was a problem hiding this comment.
I'm hesitant here. You are right in the sense that we do that everywhere but to me it makes little sense really.
I think it would be better to have a set of all coins so that we can just iterate over it and that's it. It does not add anything to the memory because we would just store pointers.
Otherwise, it seems like we create the set of all coins again and again.
There was a problem hiding this comment.
I agree that it makes little sense; we can introduce it here and change it in other places? I believe that in classes like this one, consistency is really important so it's easier to understand.
There was a problem hiding this comment.
I agree that it makes little sense; we can introduce it here and change it in other places? I believe that in classes like this one, consistency is really important so it's easier to understand.
It would be great to fix it consistently everywhere or as much as possible. Yes.
| IEnumerable<SmartCoin> Generator(SmartCoin parentCoin, bool addSelf) | ||
| { | ||
| IEnumerable<SmartCoin> childrenOf = parentCoin.SpenderTransaction is not null | ||
| ? allCoins.Where(x => x.TransactionId == parentCoin.SpenderTransaction.GetHash()) // Inefficient. |
There was a problem hiding this comment.
This would be much cleaner and more efficient with #11516 's factoring. Anyway.
There was a problem hiding this comment.
Could we just add the dictionary you had in that PR? As a small atomic PR?
There was a problem hiding this comment.
Could we just add the dictionary you had in that PR? As a small atomic PR?
I will look into it, the smallest PR possible with the dictionary. This structure makes a lot of sense because the values are by tx anyway.
I think it's a good idea, I'll try to address that. |
kiminuo
force-pushed
the
feature/2023-10-14-Simplify-ICoinsView
branch
from
3ac3519
to
9433aaf
Compare
kiminuo
changed the title
CoinsRegistry: Refactor DescendantOfAndSelfNoLockCoinsRegistry: Refactor DescendantOf
It's imho a similar case to #11696 (review). I'm working on testing that for that 3rd party project. And other than that, we need to try. |
No description provided.