[release/1.3.1] [CO-354] Introduce `NetworkMagic` #3558

mhuesch · 2018-09-07T05:20:27Z

Description

Building on the previous commit, we add the NetworkMagic datatype,
which allows us to discriminate addresses originating from different
networks.

In order to make this commit as simple and easy-to-review as possible,
I stub out the added arguments of type NetworkMagic with a dummy
variable: fixedNM. This puts off the (fairly involved) work of
threading NetworkMagic values from the nearest occurrence of
ProtocolMagic to where said values are needed.

The remainder of the work will come in a separate commit.

Linked issue

https://iohk.myjetbrains.com/youtrack/issue/CO-354

Type of change

[~] 🐞 Bug fix (non-breaking change which fixes an issue)
🛠 New feature (non-breaking change which adds functionality)
[~] ⚠️ Breaking change (fix or feature that would cause existing functionality to change)
[~] 🏭 Refactoring that does not change existing functionality but does improve things like code readability, structure etc
[~] 🔨 New or improved tests for existing code
[~] ⛑ git-flow chore (backport, hotfix, etc)

Developer checklist

I have read the style guide document, and my code follows the code style of this project.
If my code deals with exceptions, it follows the guidelines.
I have updated any documentation accordingly, if needed. Documentation changes can be reflected in opening a PR on cardanodocs.com, amending the inline Haddock comments, any relevant README file or one of the document listed in the docs directory.
[~] CHANGELOG entry has been added and is linked to the correct PR on GitHub.
^ there is no 1.3.1 section in the CHANGELOG, and this is dev work on a release branch, so I'm skipping this.

Testing checklist

[~] I have added tests to cover my changes.
All new and existing tests passed.
^ no tests were added because these changes introduce new capabilities which are not used yet. Thus the behavior is the same, and the existing tests should be adequate.

mhuesch · 2018-09-07T06:16:43Z

N.B. this PR contains commits which are part of PR #3556. Thus, the "Files Changed" is inflated, and if someone reviews this before #3556 is merged, it would be simplest to review only the "Introduce NetworkMagic" commit.

EDIT:
I, @intricate, have rebased this branch on release/1.3.1 after the merging of #3556 so this comment no longer applies.

mhuesch · 2018-09-07T06:49:05Z

core/src/Pos/Core/NetworkMagic.hs

+                    b2 = fromIntegral $ shift (shift w  8) (-24)
+                    b3 = fromIntegral $ shift (shift w 16) (-24)
+                    b4 = fromIntegral $ shift (shift w 24) (-24)
+                 in b1 + b2 + b3 + b4


We should make a decision about whether we should use a proper hash function instead of this ad-hoc checksum.

@mhuesch I'm in agreement with using a standard hash or checksum function.

parsonsmatt · 2018-09-10T19:48:16Z

auxx/src/Command/Tx.hs

@@ -272,3 +273,6 @@ sendTxsFromFile diffusion txsFile = do
                (topsortTxAuxes txAuxes)
        let submitOne = submitTxRaw diffusion
        mapM_ submitOne sortedTxAuxes
+
+fixedNM :: NetworkMagic
+fixedNM = NMNothing


Why is this term repeated instead of defined once and imported?

That is removed in a subsequent PR.

@parsonsmatt this was defined multiple times so that we could more easily track which files had the hardcoded variable. This outlined the frontier where we needed configuration passed to.

They were removed in #3561

parsonsmatt · 2018-09-10T19:50:31Z

core/src/Pos/Core/NetworkMagic.hs

+-- mhueschen: although it makes no sense for an identifier to have a sign, we're
+-- opting to maintain consistency with `ProtocolMagicId`, rather than risk subtle
+-- conversion bugs.
+data NetworkMagic


I really don't like this type name. What is Magic? What does it mean, what does it do, why do we care about it?

"Magic" is fairly idiomatic.

A constant numerical or text value used to identify a file format or protocol;

It's the magic number which identifies a blockchain. I'd say a better name would be BlockchainMagic, since "network" has other connotations. It has nothing to do with the communication network.

Open to modifying this. NetworkMagic was a suggestion from @dcoutts which we ran with.

No. NetworkMagic is probably the best name for it.

parsonsmatt · 2018-09-10T19:51:18Z

core/test/Test/Pos/Core/Arbitrary.hs

@@ -253,6 +254,9 @@ instance Arbitrary AddrStakeDistribution where
                        CoinPortion <$> choose (1, max 1 (limit - 1))
                    genPortions (n - 1) (portion : res)

+instance Arbitrary NetworkMagic where
+    arbitrary = oneof [pure NMNothing, NMJust <$> arbitrary]


This will use the arbitrary of Int32 which permits negative values. Is that what we want here?

The type should probably be Word32.

ProtocolMagic uses Int32, which I agree is a bad choice and should be Word32. I'm fairly sure that fromIntegral will do the conversion we want, but I also worry that we could be bitten by mixing Word32/Int32 types in ProtocolMagic and NetworkMagic.

parsonsmatt · 2018-09-10T19:53:09Z

core/src/Pos/Core/NetworkMagic.hs

+
+-- mhueschen: although it makes no sense for an identifier to have a sign, we're
+-- opting to maintain consistency with `ProtocolMagicId`, rather than risk subtle
+-- conversion bugs.


We should be as correct as possible and provide small locally safe functions for conversion. In this case, providing NMJust !Word32 would be Right, and we could have a function ProtocolMagicId -> NetworkMagic that handles it correctly.

Alternatively, a smart constructor like x :: Int32 -> Maybe NetworkMagic that fails on < 0.

I believe we want negative values. The type of ProtocolMagic's identifier should really be a Word32, but for historical reasons is an Int32. The point of the identifier is to disambiguate networks, so negative values also serve that purpose.

parsonsmatt · 2018-09-10T19:53:35Z

core/test/Test/Pos/Core/Arbitrary/Unsafe.hs

                }
        let addrType = ATPubKey
        return Address {..}

 instance HasProtocolConstants => ArbitraryUnsafe SlotId where
    arbitraryUnsafe = SlotId <$> arbitraryUnsafe <*> arbitraryUnsafe
+
+instance ArbitraryUnsafe NetworkMagic


This will also have negative ints in the field due to generic deriving.

See #3558 (comment) - I think this is OK.

parsonsmatt · 2018-09-10T19:53:50Z

core/test/Test/Pos/Core/ExampleHelpers.hs

+                                  SharedSeed (..), SlotLeaders, StakeholderId, StakesList,
+                                  TxFeePolicy (..), TxSizeLinear (..), addressHash,
+                                  coinPortionDenominator, makeAddress, makePubKeyAddress,
+                                  mkMultiKeyDistr)


This stylish-haskell config is wrong.

This PR is against the release/1.3.1 branch, so the stylish-haskell rules are the old ones.

^ what Erik said. It bothered me too, but I didn't want to reformat all of the imports. This particular modification occurred when @intricate moved code from develop to release/1.3.1 without re-running stylish-haskell in #3540. I added a intermediate commit to correct the style, so it's consistent with release/1.3.1.

parsonsmatt · 2018-09-10T19:54:09Z

core/test/Test/Pos/Core/Gen.hs

+
+
+fixedNM :: NetworkMagic
+fixedNM = NMNothing


Yeah this should definitely be relocated with the type definition as an alias.

#3558 (comment)

parsonsmatt · 2018-09-10T19:54:23Z

core/test/Test/Pos/Core/Json.hs

+import           Test.Pos.Util.Golden (discoverGolden, eachOf, goldenTestCanonicalJSONDec,
+                                       goldenTestJSON, goldenTestJSONDec)
+import           Test.Pos.Util.Tripping (discoverRoundTrip, roundTripsAesonBuildable,
+                                         roundTripsAesonShow, roundTripsCanonicalJSONShow)


you need to fix your stylish-haskell config :)

#3558 (comment)

erikd · 2018-09-10T20:50:46Z

core/src/Pos/Core/NetworkMagic.hs

+-- conversion bugs.
+data NetworkMagic
+    = NMNothing
+    | NMJust !Int32


Should probably be Word32

I agree. However I'm a little scared about making ProtocolMagic and NetworkMagic have different types. #3558 (comment)

parsonsmatt · 2018-09-10T22:10:22Z

Thanks for the clarifications; PR approved :)

mhuesch · 2018-09-10T22:31:47Z

@parsonsmatt thanks for the review!

Building on the previous commit, we add the `NetworkMagic` datatype, which allows us to discriminate addresses originating from different networks. In order to make this commit as simple and easy-to-review as possible, I stub out the added arguments of type `NetworkMagic` with a dummy variable: `fixedNM`. This puts of the (fairly involved) work of threading `NetworkMagic` values from the nearest occurrence of `ProtocolMagic` to where said values are needed. The remainder of the work will come in a separate commit. --- Merged with: [CO-354] Make use of Blake2b_256 for constructing NetworkMagic [CO-354] NetworkMagic: use Int32 rather than Word8 We opt to keep the full 32bits of the ProtocolMagicId, instead of truncating to 8 bits. This reduces the probability of collisions. While we would ideally use a Word32 (since sign has no meaning in identifier-space), we're stuck using Int32 because that is how ProtocolMagicId is defined. --- Co-authored-by: Michael Hueschen <michael.hueschen@iohk.io> Co-authored-by: Luke Nadur <luke.nadur@iohk.io>

mhuesch requested review from adinapoli-iohk, akegalj, denisshevchenko, erikd, ksaric, parsonsmatt and vincenthz as code owners September 7, 2018 05:20

mhuesch removed request for vincenthz, akegalj, denisshevchenko, ksaric and adinapoli-iohk September 7, 2018 05:21

mhuesch self-assigned this Sep 7, 2018

mhuesch mentioned this pull request Sep 7, 2018

[release/1.3.1] [CO-354] Split test-suites for NMNothing/NMJust #3559

Merged

7 tasks

mhuesch commented Sep 7, 2018

View reviewed changes

mhuesch mentioned this pull request Sep 7, 2018

[release/1.3.1] [CO-354] Implement NetworkMagic logic #3561

Merged

6 tasks

mhuesch added the DO NOT MERGE label Sep 7, 2018

intricate requested a review from ruhatch September 7, 2018 15:53

intricate force-pushed the intricate+mhuesch/CO-354/introduce-networkmagic branch from 1ba4152 to dcbaef3 Compare September 10, 2018 12:26

mhuesch force-pushed the intricate+mhuesch/CO-354/introduce-networkmagic branch 2 times, most recently from 638ec44 to d08468f Compare September 10, 2018 17:56

[CO-354] Fix stylish-haskell

bad8163

mhuesch force-pushed the intricate+mhuesch/CO-354/introduce-networkmagic branch from d08468f to 8b43db7 Compare September 10, 2018 17:58

parsonsmatt suggested changes Sep 10, 2018

View reviewed changes

erikd reviewed Sep 10, 2018

View reviewed changes

mhuesch removed the DO NOT MERGE label Sep 10, 2018

parsonsmatt previously approved these changes Sep 10, 2018

View reviewed changes

mhuesch dismissed parsonsmatt’s stale review via b12682e September 10, 2018 23:07

mhuesch force-pushed the intricate+mhuesch/CO-354/introduce-networkmagic branch from 8b43db7 to b12682e Compare September 10, 2018 23:07

erikd approved these changes Sep 11, 2018

View reviewed changes

intricate approved these changes Sep 11, 2018

View reviewed changes

adinapoli-iohk merged commit 89e651a into release/1.3.1 Sep 11, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[release/1.3.1] [CO-354] Introduce `NetworkMagic` #3558

[release/1.3.1] [CO-354] Introduce `NetworkMagic` #3558

mhuesch commented Sep 7, 2018

mhuesch commented Sep 7, 2018 •

edited by intricate

Loading

mhuesch Sep 7, 2018

intricate Sep 7, 2018

parsonsmatt Sep 10, 2018

erikd Sep 10, 2018

mhuesch Sep 10, 2018

parsonsmatt Sep 10, 2018

avieth Sep 10, 2018

mhuesch Sep 10, 2018

erikd Sep 10, 2018

parsonsmatt Sep 10, 2018

erikd Sep 10, 2018

mhuesch Sep 10, 2018

parsonsmatt Sep 10, 2018

mhuesch Sep 10, 2018

parsonsmatt Sep 10, 2018

mhuesch Sep 10, 2018

parsonsmatt Sep 10, 2018

erikd Sep 10, 2018

mhuesch Sep 10, 2018 •

edited

Loading

parsonsmatt Sep 10, 2018

mhuesch Sep 10, 2018

parsonsmatt Sep 10, 2018

mhuesch Sep 10, 2018

erikd Sep 10, 2018

mhuesch Sep 10, 2018

parsonsmatt commented Sep 10, 2018

mhuesch commented Sep 10, 2018



		fixedNM :: NetworkMagic
		fixedNM = NMNothing

[release/1.3.1] [CO-354] Introduce NetworkMagic #3558

[release/1.3.1] [CO-354] Introduce NetworkMagic #3558

Conversation

mhuesch commented Sep 7, 2018

Description

Linked issue

Type of change

Developer checklist

Testing checklist

mhuesch commented Sep 7, 2018 • edited by intricate Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhuesch Sep 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parsonsmatt commented Sep 10, 2018

mhuesch commented Sep 10, 2018

[release/1.3.1] [CO-354] Introduce `NetworkMagic` #3558

[release/1.3.1] [CO-354] Introduce `NetworkMagic` #3558

mhuesch commented Sep 7, 2018 •

edited by intricate

Loading

mhuesch Sep 10, 2018 •

edited

Loading