Checkpoints misc cleanup

theuni commented at 0:18 am on June 6, 2015: member

This is part of a much larger networking refactor that I’m working on, but it broke out cleanly so I’m submitting it independently.

Chainparams will soon be free of app state, so these are a few easy cleanups towards that end. Specifically, this drops the dependencies on checkpoints (and main as a side-effect) and boost.

in src/chainparams.cpp: in 6244dde97d outdated

93-        base58Prefixes[EXT_SECRET_KEY] = boost::assign::list_of(0x04)(0x88)(0xAD)(0xE4).convert_to_container<std::vector<unsigned char> >();
94+        const unsigned char chPubkeyAddress[] = { 0 };
95+        const unsigned char chScriptAddress[] = { 5 };
96+        const unsigned char chSecretKey[] =     { 128 };
97+        const unsigned char chExtPublicKey[] =  { 0x04, 0x88, 0xB2, 0x1E };
98+        const unsigned char chExtSecretKey[] =  { 0x04, 0x88, 0xAD, 0xE4 };

laanwj commented at 7:59 am on June 6, 2015:

I vaguely remember that we already went through this change once, but the other way around. The point to use boost here is that it easily transitions to c++11 lingo. I’d like to leave this alone until we do.

laanwj commented at 8:07 am on June 6, 2015:

Here,

only two years ago this was all changed to list_of in the name of newness: 8388289eb6dfc9add233f8f8b536f84caf771fa6
Then you changed it to get rid of the global namespace 856e862f4a736fbdc38daae3b7f0fa34e1da317c
Then some list_ofs had to be changed to std::vector again a2b04ddfe6452e7d6274f4096bf3f2aee695a6d9 in the name of c++11

… and now we’re full circle, back to C-style initialization. Sorry for this, this may be just a personal annoyance against flip-flopping perfectly fine code, but I feel this is not helping anyone :)

theuni commented at 5:55 pm on June 6, 2015:

Well in fairness, the last two steps reduced the boost dependency, this just goes the final step and actually removes it. I agree that the gradual touching and “improving” can get annoying, but I don’t think there’s been any flip-flopping here, just small steps.

I understand if you’d rather hold on this and use initializer lists instead, but I may pick up this fight again if this would be usable in a lib before we move to c++11 :)

Edit: whoops, dangling text removed.

jtimon commented at 1:47 pm on June 21, 2015:

I was also removing boost dependencies from consensus code, but now I am not sure what is preferred. Chainparams is out of consensus and it seems reasonable to move towards c++11. But maybe we want to eventually have a libconsensus not only exposed but also written in C? In any case, I would really like to have a more clear criterion. My idea was “remove boost when possible”, then @laanwj told be that boost/foreach was there for later c++11, and I thought that was an exception. But now I’m really confused and I definitely want to avoid going in circles.

in src/chainparams.cpp: in 6244dde97d outdated

121-            (225430, uint256S("0x00000000000001c108384350f74090433e7fcf79a606b8e797f065b130575932"))
122-            (250000, uint256S("0x000000000000003887df1f29024b06fc2200b55f8af8f35453d7be294df2d214"))
123-            (279000, uint256S("0x0000000000000001ae8c72a0b0c301f67e3afca10e819efa9041e458e9bd7e40"))
124-            (295000, uint256S("0x00000000000000004d9b4ef50f0f9d686fd69db2e03af35a100370c64632a983")),
125+        const MapCheckpoints::value_type rawCheckpoints[] = {
126+            std::make_pair( 11111, uint256S("0x0000000069e244f73d78e8fd29ba2fd2ed618bd6fa2ee92559f542fdb26e7c1d")),

laanwj commented at 8:01 am on June 6, 2015:

Same here. Just like with FOREACH, getting rid of simple boost header-only helpers is IMO not a goal, unless they can be transitioned to the appropriate c++11 constructs (which at least is a clear way forwards).

theuni commented at 5:59 pm on June 6, 2015:

It’s certainly a goal for me if it means that boost would be required for a lib. I have no problem with boost in app-level code, but I couldn’t justify requiring boost for a lib just for syntactic sugar.

theuni commented at 6:00 pm on June 6, 2015:

To clarify the above, I’ll certainly concede that point and revert this for now, but I’d like to revisit if that scenario arises.

laanwj commented at 6:47 am on June 7, 2015:

But my point is, and has been for years, that using syntactic sugar such as FOREACH makes sense not because I love boost but because c++11 defines similar constructs. When the source code is (finally) switched over this makes them easy to replace. I’m so tired of making this argument, though, that I hope we switch to c++11 soon and leave this behind us.

laanwj added the label Refactoring on Jun 9, 2015

sipa commented at 2:55 pm on June 16, 2015: member

utACK for me, with or without deboostification, but needs rebase.

jtimon commented at 1:48 pm on June 21, 2015: contributor

Yes, untested ACK apart from the boost discussion.

sipa commented at 1:51 pm on July 9, 2015: member

Still needs rebase.

Diapolo commented at 3:29 pm on July 9, 2015: none

@sipa AFAIK @theuni Is travelling currently.

chainparams: move CCheckpointData into chainparams.h

This unties CChainParams from its dependency on checkpoints. Instead, now it
only depends on the raw checkpoint data.

f0deec572b

theuni force-pushed on Jul 28, 2015

chainparams: don't use std namespace 17221bf77e

theuni commented at 7:49 pm on July 28, 2015: member

Rebased and dropped the boost change.

jtimon commented at 11:15 am on July 29, 2015: contributor

re-utACK.

laanwj commented at 4:06 pm on August 20, 2015: member

ACK

laanwj merged this on Aug 20, 2015

laanwj closed this on Aug 20, 2015

laanwj referenced this in commit e3f13ddc54 on Aug 20, 2015

in src/chainparams.h: in 17221bf77e

22@@ -24,6 +23,14 @@ struct SeedSpec6 {
23     uint16_t port;
24 };
25 
26+typedef std::map<int, uint256> MapCheckpoints;

dcousens commented at 10:00 pm on August 20, 2015:

Is the typedef needed?

jtimon commented at 10:17 pm on August 20, 2015:

It is true that it doesn’t add much readability at this point. Maybe you can fix it with a commit to the trivial branch?

dcousens commented at 10:29 pm on August 20, 2015:

@theuni would need to rebase said branch to include this.

zkbot referenced this in commit e1c68f0631 on Jan 19, 2018

zkbot referenced this in commit 4f77ce5cb1 on Jan 22, 2018

zkbot referenced this in commit cc571a3ccd on Jan 22, 2018

zkbot referenced this in commit 50c23880c7 on Jan 22, 2018

zkbot referenced this in commit a4a020de7b on Jan 22, 2018

DrahtBot locked this on Sep 8, 2021

Checkpoints misc cleanup #6242