Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

- In case of reorganized block the Bech32UtxoSet recovered to its previous state #48

Merged
merged 1 commit into from
Mar 5, 2018
Merged

- In case of reorganized block the Bech32UtxoSet recovered to its previous state #48

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
74 changes: 70 additions & 4 deletions MagicalCryptoWallet/Services/IndexBuilderService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,67 @@ public class IndexBuilderService
private List<FilterModel> Index { get; }
private AsyncLock IndexLock { get; }

private Dictionary<OutPoint, Script> Bech32UtxoSet { get; }
private Dictionary<OutPoint, Script> Bech32UtxoSet { get; } //Change to observable concurrent dictionary? https://www.nuget.org/packages/ParallelExtensionsExtras/
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While concurrent collections don't throw exceptions, not all threading issues can be ignored. And they can work in a quite misleading way: https://stackoverflow.com/a/1966462/2061103

I want it to throw an exception if something is wrong. As you see, right above this line there is an IndexLock for the Index list. This is, because the Index may encounter threading issues, but as the code is today Bech32UtxoSet won't encounter threading issues, since it's an "internal database" that's being touched in a proper way.

Also performance is quite an issue here, so that's another con against concurrent collections.

Generally I use concurrent collections against lock, whenever

  1. I'm lazy.
  2. I'm showing data to the user (that doesn't have to be so accurate.)

Discussion on this topic can be found here: stratisproject/StratisBitcoinFullNode#617

If you agree, please remove the todo comment here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure I will remove. P.S.: I would use the observable part of the dictionary to subcribe the add/remove/clear event to ensure that all changes of the dictionary get noticed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another way is to implement my own observable dictionary without concurrency: something like this
https://stackoverflow.com/questions/5663395/net-observabledictionary

Copy link
Owner

@nopara73 nopara73 Mar 5, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's better as it is now, since it's simpler, if it gets more complex we can change. Btw, I created my ObservableConcurrentDictionary if you ever need such: https://github.com/nopara73/HiddenWallet/blob/cc8c948ac7a1b9173610145e60c303d71a8c0a66/HiddenWallet/Helpers/ConcurrentObservableDictionary.cs




internal static class ActionHistoryHelper
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you make it non-static? While it'll be a singleton for now, later or during tests it could cause problems.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a very special class only for this special purpose. I do not want anybody to create more instances or make it visible from outside.
Of course I can change it to non static.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please make it. My concern is if IndexBuilderService becomes used in different places it'll break. In fact it probably will while testing. Tests are usually running concurrently, and back then my static global classes were the cause of my unreliable tests, so I'd like to avoid it this time.

I do not want anybody to create more instances or make it visible from outside.

I guess you can make it private, too. If you don't want to write tests for it. I think tests are not needed.

{
public enum Operation
{
Add,
Remove
}
private static readonly List<ActionItem> ActionHistory = new List<ActionItem>();

public class ActionItem
{
public Operation Action { get; }
public OutPoint OutPoint { get; }
public Script Script { get; }

public ActionItem(Operation action, OutPoint outPoint, Script script )
{
Action = action;
OutPoint = outPoint;
Script = script;
}
}

public static void ClearActionHistory()
{
ActionHistory.Clear();
}

public static void StoreAction(ActionItem actionItem)
{
ActionHistory.Add(actionItem);
}
public static void StoreAction(Operation action, OutPoint outpoint, Script script)
{
StoreAction(new ActionItem(action, outpoint, script));
}

public static void Rollback(Dictionary<OutPoint, Script> toRollBack)
{
for (var i = ActionHistory.Count-1; i >= 0; i--)
{
ActionItem act = ActionHistory[i];
switch (act.Action)
{
case Operation.Add:
toRollBack.Remove(act.OutPoint);
break;
case Operation.Remove:
toRollBack.Add(act.OutPoint,act.Script);
break;
default:
throw new ArgumentOutOfRangeException();
}
}
ActionHistory.Clear();
}
}

public Height StartingHeight // First possible bech32 transaction ever.
{
Expand Down Expand Up @@ -146,7 +206,6 @@ public void Syncronize()

if (prevHash != null)
{
// ToDo: IMPORTANT! The Bech32UtxoSet MUST be recovered to its previous state, too!
if (prevHash != block.Header.HashPrevBlock) // reorg
{
using (await IndexLock.LockAsync())
Expand All @@ -157,10 +216,13 @@ public void Syncronize()
// remove last line
var lines = File.ReadAllLines(IndexFilePath);
File.WriteAllLines(IndexFilePath, lines.Take(lines.Length - 1).ToArray());
continue;
ActionHistoryHelper.Rollback(Bech32UtxoSet); //The Bech32UtxoSet MUST be recovered to its previous state
continue; //skip the current block
}
}

ActionHistoryHelper.ClearActionHistory(); //reset history.

var scripts = new HashSet<Script>();

foreach (var tx in block.Transactions)
Expand All @@ -170,7 +232,9 @@ public void Syncronize()
var output = tx.Outputs[i];
if (!output.ScriptPubKey.IsPayToScriptHash && output.ScriptPubKey.IsWitness)
{
Bech32UtxoSet.Add(new OutPoint(tx.GetHash(), i), output.ScriptPubKey);
var outpoint = new OutPoint(tx.GetHash(), i);
Bech32UtxoSet.Add(outpoint, output.ScriptPubKey);
ActionHistoryHelper.StoreAction(ActionHistoryHelper.Operation.Add, outpoint, output.ScriptPubKey);
scripts.Add(output.ScriptPubKey);
}
}
Expand All @@ -180,7 +244,9 @@ public void Syncronize()
var found = Bech32UtxoSet.SingleOrDefault(x => x.Key == input.PrevOut);
if (found.Key != default)
{
Script val=Bech32UtxoSet[input.PrevOut];
Bech32UtxoSet.Remove(input.PrevOut);
ActionHistoryHelper.StoreAction(ActionHistoryHelper.Operation.Remove, input.PrevOut, val);
scripts.Add(found.Value);
}
}
Expand Down