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.

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

Allow for programmatic configuration of protocol settings #845

Merged
merged 4 commits into from Jun 20, 2019

Conversation

7 participants
@devhawk
Copy link
Contributor

commented Jun 18, 2019

This PR adds a mechanism to configure the protocol settings programmatically. This enhancement is needed to enable automatic configuration of the privatenet blockchain,

This change adds an Initialize static function to ProtocolSettings, enabling the host process to pass custom protocol configuration information without needing to write a protocol.json file to disk. ASP.NET Configuration already supports multiple configuration providers. The caller of Initialize is free to use whatever configuration provider they wish, including in-memory .NET objects.

If Initialize method is not called, the first time the ProtocolSettings.Default get function is called, protocol configuration is loaded exactly as it loaded today - the protocol.json file is used as a configuration provider if available with mainnet defaults used for any configuration settings not provided. Again, as today these settings are cached for later use.

ProtocolSettings.Initialize must be called before the ProtocolSettings.Default property is ever accessed. Initialize will not overwrite existing loaded protocol settings - regardless if those settings were loaded by an earlier call to Initialize or by the first time the Default property is accessed. CompareExchange is used to ensure protocol settings are only initialized once, even in the face of multiple threads possibly accessing ProtocolSettings.

PR also includes unit tests for ProtocolSettings initialization. All unit tests pass locally.

@vncoelho
Copy link
Member

left a comment

Such a flexible and well designed PR, @devhawk.
Great job with specific use of C# functions...aehuahuea

As soon as we have NEO 3.0 first preview we can validate this in practice.
PS: I restarted the TRAVIS job because UT looks pass locally as well.

@steven1227

This comment has been minimized.

Copy link

commented Jun 19, 2019

This is a great Pull Request @devhawk

@erikzhang

This comment has been minimized.

Copy link
Member

commented Jun 19, 2019

I can't imagine how to switch networks without downtime after a node is running.

IConfigurationSection section = new ConfigurationBuilder().AddJsonFile("protocol.json", true).Build().GetSection("ProtocolConfiguration");
Default = new ProtocolSettings(section);
var settings = new ProtocolSettings(configuration.GetSection("ProtocolConfiguration"));
return null == Interlocked.CompareExchange(ref _default, settings, null);

This comment has been minimized.

Copy link
@shargon

shargon Jun 19, 2019

Member

When it return true? only when change the first time?

This comment has been minimized.

Copy link
@devhawk

devhawk Jun 19, 2019

Author Contributor

Yes, UpdateDefault will only return true the first time it was called. CompareExchange atomically compares _default to null and stores settings in _default if and only if the comparison succeeds.

I agree with @erikzhang - a node can't switch networks once it is up and running. This code ensures _default's value can only be written once - even if there are multiple threads in the process accessing ProtocolSettings.Default.

@codecov-io

This comment has been minimized.

Copy link

commented Jun 19, 2019

Codecov Report

Merging #845 into master will increase coverage by 0.05%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #845      +/-   ##
==========================================
+ Coverage   38.39%   38.45%   +0.05%     
==========================================
  Files         176      176              
  Lines       12465    12475      +10     
==========================================
+ Hits         4786     4797      +11     
+ Misses       7679     7678       -1
Impacted Files Coverage Δ
neo/ProtocolSettings.cs 94% <100%> (+4%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 70bf2f5...36af0d6. Read the comment docs.

return null == Interlocked.CompareExchange(ref _default, settings, null);
}

public static bool Initialize(IConfiguration configuration)

This comment has been minimized.

Copy link
@erikzhang

erikzhang Jun 19, 2019

Member

Do we need this Initialize()? UpdateDefault() will be called automatically when accessing Default.

This comment has been minimized.

Copy link
@devhawk

devhawk Jun 19, 2019

Author Contributor

Initialize is a public method intended to be called by host code. UpdateDefault is a private helper method that does the actual CompareExchange so that I didn't Duplicate code in the Initialize and Default getter code paths.

This comment has been minimized.

Copy link
@igormcoelho

igormcoelho Jun 19, 2019

Contributor

This is very useful for testing. This way we can manage to set everything up (using Initialize() before actually starting the other components. We could apply a similar solution to neo-plugins Settings: https://github.com/neo-project/neo-plugins/blob/a72c44813271efcafe0a4fcee2505fae3a3b4716/ApplicationLogs/Settings.cs#L9

@igormcoelho
Copy link
Contributor

left a comment

Fine for me. As long as it's used once, and before network initialization.
Tests pass, but in the future we need to test this in a more integrated way (perhaps the DI approach @rodoufu).

@erikzhang erikzhang merged commit e844108 into neo-project:master Jun 20, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.