-
Notifications
You must be signed in to change notification settings - Fork 382
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
parametrize genesis block #988
Conversation
The build script only updates build artifacts. It cannot verify the compile or test execution - that's up to the automated build platforms. So you could have broken a VS or other platform build, but at least the project files are correct. |
My understanding was that the project files are the Makefiles of the VS universe. If Makefiles are outdated then the build can break. But of course it can break for other reasons too 😉 |
Right, so the script only ensures that the project and make files are generated for current sources and metadata, you can still break the VS build. |
src/settings.cpp
Outdated
{ | ||
case config::settings::mainnet: | ||
{ | ||
encoded_genesis_block = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The settings pattern uses serializable classes to read settings to produce the corresponding object. I think the proper generalization here is actually a chain::block.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would make the chain::block::genesis()
function obsolete. I'll remove it then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It makes it redundant, but that’s fine. It can accept only the standard config enumeration and construct the well known instances, as it does now, and those can be used to simplify the standard settings init. Or that can be done in reverse, which I think is better since the values remain with the standard settings initializer.
include/bitcoin/bitcoin/settings.hpp
Outdated
@@ -48,6 +51,8 @@ class BC_API settings | |||
|
|||
// The target number of blocks for 2 weeks of work (2016 blocks). | |||
size_t retargeting_interval; | |||
|
|||
std::string encoded_genesis_block; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
chain::block genesis_block
@@ -200,7 +140,7 @@ bool block::operator!=(const block& other) const | |||
//----------------------------------------------------------------------------- | |||
|
|||
// static | |||
block block::factory(const data_chunk& data, const settings& settings, | |||
block block::factory(const data_chunk& data, const bc::settings& settings, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why all of the bc::
namespace qualifications, where is the conflict?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It conflicts with bc::config::settings
.
examples/main.cpp
Outdated
@@ -45,7 +45,8 @@ int bc::main(int argc, char* argv[]) | |||
#endif | |||
|
|||
// Extracting Satoshi's words from genesis block. | |||
const auto block = bc::chain::block::genesis_mainnet(bc::settings()); | |||
const auto block = | |||
bc::settings(bc::config::settings::mainnet).genesis_block; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style is to generally break the line, not move it to another line (as this is not an option if the line is just as bit longer, so cannot be done consistently).
const auto block = bc::settings(bc::config::settings::mainnet)
.genesis_block;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will fix.
include/bitcoin/bitcoin/settings.hpp
Outdated
@@ -31,6 +34,9 @@ class BC_API settings | |||
{ | |||
public: | |||
settings(); | |||
settings(config::settings context); | |||
|
|||
void genesis(const std::string& encoded_genesis_block); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be private.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will go away as you suggested below.
src/settings.cpp
Outdated
{ | ||
} | ||
|
||
settings::settings(config::settings context) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The use of text deserialization internal to the settings instance is inconsistent. Setting and command line deserialization is implemented in bc::config
classes, often encapsulating the primitive. Additionally, use of genesis(text) below is unnecessary. There is no reason to construct the object from a text string. The text stream is broken up into (sort of) logical values for readability. But this is much more clearly and safely accomplished using the chain::block
constructor as it is intended. Comparing the result to a well-known text stream and block hash result is the job of two text cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have removed the genesis()
helper method. I will add the deserialization class in bc::config
in an upcoming PR to prevent this one from growing too much.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the input. Will continue tomorrow.
@@ -200,7 +140,7 @@ bool block::operator!=(const block& other) const | |||
//----------------------------------------------------------------------------- | |||
|
|||
// static | |||
block block::factory(const data_chunk& data, const settings& settings, | |||
block block::factory(const data_chunk& data, const bc::settings& settings, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It conflicts with bc::config::settings
.
src/settings.cpp
Outdated
{ | ||
} | ||
|
||
settings::settings(config::settings context) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will fix.
examples/main.cpp
Outdated
@@ -45,7 +45,8 @@ int bc::main(int argc, char* argv[]) | |||
#endif | |||
|
|||
// Extracting Satoshi's words from genesis block. | |||
const auto block = bc::chain::block::genesis_mainnet(bc::settings()); | |||
const auto block = | |||
bc::settings(bc::config::settings::mainnet).genesis_block; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will fix.
include/bitcoin/bitcoin/settings.hpp
Outdated
@@ -31,6 +34,9 @@ class BC_API settings | |||
{ | |||
public: | |||
settings(); | |||
settings(config::settings context); | |||
|
|||
void genesis(const std::string& encoded_genesis_block); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will go away as you suggested below.
9b200ba
to
716142e
Compare
see: #997 |
The libbitcoin-build script didn't change anything so I'm assuming that I didn't break the VS build. I'm fixing the other repos now.