MetacoSA / NBitcoin

Comprehensive Bitcoin library for the .NET framework.
MIT License
1.87k stars 845 forks source link

Update Network GetDefaultDataFolder to fallback to OS Temp folder when HOME and APPDATA not available #1063

Closed augustoproiete closed 2 years ago

augustoproiete commented 2 years ago

Fallback to the operating system's TEMP folder when both HOME and APPDATA are not available, as a workaround.

A better approach for a future improvement would be to remove any filesystem dependencies when accessing networks.


Repro steps: https://github.com/augustoproiete-repros/nbitcoin-issue-1050-ubuntu-exception

Network.GetDefaultDataFolder expects either HOME or APPDATA environment variables to be available, which might not be the case in some scenarios as described in https://github.com/MetacoSA/NBitcoin/issues/1050 in which case it returns null.

CreateSignet relies on Network.GetDefaultDataFolder to set the path of the signet cookie and currently throws an exception when the value is null.

image


Closes https://github.com/MetacoSA/NBitcoin/issues/1050

knocte commented 2 years ago

But reverting to the tmp folder might let the user think that their data is safe while it's not. IMHO the better way to solve this is an optional DirectoryInfo parameter in InitSignet() maybe?

knocte commented 2 years ago

that their data is safe while it's not.

Mmm or maybe signet related data is no big deal to lose because it's not big anyway?

augustoproiete commented 2 years ago

an optional DirectoryInfo parameter in InitSignet() maybe?

The issue is that the code runs in a static context during initialization, before the user had the opportunity to send any parameters (it's a private constructor).

Perhaps a better approach would be to introduce an "Options" class with some defaults, which the user can override?

Something along those lines:

var myBitcoinOptions = new BitcoinOptions
{
    DataFolderPath = "./myapp/bitcoin-data",
};

Bitcoin.DefaultOptions = () => myBitcoinOptions;

// Static context relies on Bitcoin.DefaultOptions().DataFolderPath
Network network = Network.Main;

// ---

public class BitcoinOptions
{
    public string DataFolderPath { get; set; }
}

public class Bitcoin
{
    public static Func<BitcoinOptions> DefaultOptions { get; set; } = () => new BitcoinOptions();
}

We could then update the code to rely on the DataFolderPath of these options, and throw an exception if we couldn't detect the HOME/APPDATA path, so that the user knows they have to set it.

knocte commented 2 years ago

Ughh so ugly lol, I prefer the GetTempPath() approach better now 😅

NicolasDorier commented 2 years ago

I did this e4a0b4baa10b4f4c97523aa321d59c7653679f30 instead.

As I was checking how GetDefaultDataFolder is currently used, I noticed other clients of this method, handle the case when it's null. So I followed this.