Allow use of large chain ids #1289

jframe · 2019-04-16T03:56:05Z

PR description

This updates chainId to be a BigInteger so that large values for a chainId can be used. Decided on using BigInteger as Geth currently using BigInt.

Fixed Issue(s)

…Integer

ajsutton · 2019-04-16T10:20:21Z

ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/core/Transaction.java

+      recId = v.subtract(REPLAY_UNPROTECTED_V_BASE).byteValue();
+    } else if (v.compareTo(REPLAY_PROTECTED_V_MIN) > 0) {
+      chainId = v.subtract(REPLAY_PROTECTED_V_BASE).divide(TWO);
+      recId = v.subtract(TWO.multiply(chainId).add(REPLAY_PROTECTED_V_BASE)).byteValue();


Probably should use byteValueExact instead of byteValue here and line 96.

ajsutton · 2019-04-16T10:27:05Z

ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/core/Transaction.java

-    } else if (v > REPLAY_PROTECTED_V_MIN) {
-      chainId = (v - REPLAY_PROTECTED_V_BASE) / 2;
-      recId = (byte) (v - (2 * chainId + REPLAY_PROTECTED_V_BASE));
+    BigInteger chainId = BigInteger.valueOf(-1);


This probably shouldn't be using -1 as a sentinel value now that we're not using a primitive. Probably should be Optional<BigInteger> chainId = Optional.empty(); and that can flow through to Builder.chainId.

ajsutton · 2019-04-16T10:29:07Z

ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/core/Transaction.java

@@ -426,9 +429,9 @@ public String toString() {

    protected Address sender;

-    protected int chainId = -1;
+    protected BigInteger chainId = BigInteger.valueOf(-1);


Suggested change

protected BigInteger chainId = BigInteger.valueOf(-1);

protected Optional<BigInteger> chainId = Optional.empty();

ajsutton · 2019-04-16T10:29:19Z

ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/core/Transaction.java


-    public Builder chainId(final int chainId) {
+    public Builder chainId(final BigInteger chainId) {
      this.chainId = chainId;


Two possible approaches here, either public Builder chainId(final Optional<BigInteger> chainId) which is just directly assigned or public Builder chainId(final BigInteger chainId) which is wrapped with Optional.of - if you don't have a chain ID don't call chainId on the builder.

done. used approach of defaulting to Optional.empty only calling chainId on builder if you have a chainId

ajsutton · 2019-04-16T10:29:44Z

ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/core/Transaction.java

-      final OptionalInt optionalChainId =
-          chainId > 0 ? OptionalInt.of(chainId) : OptionalInt.empty();
+      final Optional<BigInteger> optionalChainId =
+          chainId.signum() == 1 ? Optional.of(chainId) : Optional.empty();


With the changes above you then don't need this and can just use chainId directly since it's already an optional.

ajsutton · 2019-04-16T10:30:53Z

...m/core/src/main/java/tech/pegasys/pantheon/ethereum/mainnet/MainnetTransactionValidator.java

@@ -37,7 +38,7 @@
 */
 public class MainnetTransactionValidator implements TransactionValidator {

-  public static final int NO_CHAIN_ID = -1;
+  public static final BigInteger NO_CHAIN_ID = BigInteger.valueOf(-1);


Shouldn't be using a sentinel value here - I suspect this constant can just go away and Optional.empty() used in its place.

ajsutton · 2019-04-16T10:31:45Z

...m/core/src/main/java/tech/pegasys/pantheon/ethereum/mainnet/MainnetTransactionValidator.java

    this.gasCalculator = gasCalculator;
    this.disallowSignatureMalleability = checkSignatureMalleability;
-    this.chainId = chainId > 0 ? OptionalInt.of(chainId) : OptionalInt.empty();
+    this.chainId = chainId.signum() == 1 ? Optional.of(chainId) : Optional.empty();


Just pass in Optional<BigInteger> and avoid the sentinel check here too.

ajsutton · 2019-04-16T10:36:48Z

pantheon/src/main/java/tech/pegasys/pantheon/cli/PantheonCommand.java

+          throw new ParameterException(
+              this.commandLine,
+              "No networkId specified and chainId in "
+                  + "genesis file is too large to be used as a networkId");


ugh, this is quite unfortunate but probably better to bail out with an error than to default to something else silently.

yeah it's not nice, but I think it's the best thing to do

CjHare · 2019-04-17T05:14:17Z

config/src/main/java/tech/pegasys/pantheon/config/ConfigUtil.java

@@ -22,4 +23,15 @@ public static OptionalLong getOptionalLong(final JsonObject jsonObject, final St
        ? OptionalLong.of(jsonObject.getLong(key))
        : OptionalLong.empty();
  }
+
+  public static BigInteger getBigInteger(final JsonObject jsonObject, final String key) {


Optional<BigInteger> for consistency with the other method in this class

rain-on · 2019-04-17T11:48:23Z

ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/core/Transaction.java

      this.chainId = chainId;
      return this;
    }

+    public Builder chainId(final BigInteger chainId) {


did you end up with 2 'chainId' setters on purpose?

just removed actually. don't think it's necessary afterall

…ng -1 as a value

…_chainId

jframe · 2019-04-18T06:28:40Z

Updated so make chainId optional in ProtocolSchedule as well. This allows having no chainId when using FixedDifficultyProtocolSchedule without the need to special -1 chainId value being used.

ajsutton

LGTM.

CjHare

LGTM

(cherry-picked from ac53e3d)

jframe added 9 commits April 15, 2019 18:15

Allow use of large chain id values by changing underlying type to Big…

216aaf2

…Integer

refactor bigInteger changes in transaction to make code more readable

063c4f3

handle large chain ids in genesis config file

b283dd8

handle large chain ids in genesis config file

3cc7c5f

integration test for large chain id

42b0d09

return error if chainId is too large to be used as a network id

1ae35f3

intg test fixes

5d6e181

Merge branch 'master' into large_chainId

8aeb6eb

change use of signum for positive number check to be consistent

a8dd51f

jframe requested review from CjHare and rain-on April 16, 2019 04:58

jframe marked this pull request as ready for review April 16, 2019 04:59

jframe requested a review from ajsutton April 16, 2019 05:33

ajsutton reviewed Apr 16, 2019

View reviewed changes

jframe added 3 commits April 17, 2019 08:45

use bytesValueExact

68e8af4

remove sentinel -1 chainId in transaction class

8e9691e

remove sentinel -1 chainId in MainnetTransactionValidator class

c4147de

CjHare reviewed Apr 17, 2019

View reviewed changes

jframe added 2 commits April 18, 2019 07:43

use a optionalBigInteger function in genesis config

eccc893

typo in chainId key name for genesis config

b124225

rain-on reviewed Apr 17, 2019

View reviewed changes

jframe added 7 commits April 18, 2019 08:31

remove unnecessary chainId method on transaction builder

9226070

Merge branch 'master' into large_chainId

0392c71

fix intg test

b521da9

Merge branch 'master' into large_chainId

8d5f250

make chainId optional in protocol spec as was but in a hidden way usi…

17e4904

…ng -1 as a value

test fixes

3765d64

Merge branch 'large_chainId' of github.com:jframe/pantheon into large…

d6b016d

…_chainId

Updated unit tests for EthChainId and NetVersion

4b25ddf

jframe and others added 3 commits April 18, 2019 17:08

spotless

a148e9d

Merge remote-tracking branch 'upstream/master' into HEAD

edf1f76

Minor updates

547223b

ajsutton approved these changes Apr 24, 2019

View reviewed changes

fix spotless

b30cb17

CjHare approved these changes Apr 24, 2019

View reviewed changes

tmohay added 3 commits April 24, 2019 21:10

update submodules

0b207dd

Merge remote-tracking branch 'upstream/master' into large_chainId

d677b61

update submodule properly

2253e7e

rain-on merged commit ac53e3d into PegaSysEng:master Apr 24, 2019

notlesh pushed a commit to notlesh/pantheon that referenced this pull request Apr 24, 2019

Allow use of large chain ids (PegaSysEng#1289)

1752a87

rain-on pushed a commit to rain-on/pantheon that referenced this pull request Apr 28, 2019

Allow use of large chain ids (PegaSysEng#1289)

c88eedf

(cherry-picked from ac53e3d)

rain-on added a commit that referenced this pull request Apr 28, 2019

Allow use of large chain ids (#1289) (#1357)

bcac581

(cherry-picked from ac53e3d)

notlesh pushed a commit to notlesh/pantheon that referenced this pull request May 4, 2019

Allow use of large chain ids (PegaSysEng#1289)

e234af7

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow use of large chain ids #1289

Allow use of large chain ids #1289

jframe commented Apr 16, 2019

ajsutton Apr 16, 2019

jframe Apr 17, 2019

ajsutton Apr 16, 2019

jframe Apr 17, 2019

ajsutton Apr 16, 2019

jframe Apr 17, 2019

ajsutton Apr 16, 2019

jframe Apr 17, 2019

ajsutton Apr 16, 2019

jframe Apr 17, 2019

ajsutton Apr 16, 2019

jframe Apr 17, 2019

ajsutton Apr 16, 2019

jframe Apr 17, 2019

ajsutton Apr 16, 2019

jframe Apr 17, 2019

CjHare Apr 17, 2019

jframe Apr 17, 2019

rain-on Apr 17, 2019

jframe Apr 17, 2019

jframe commented Apr 18, 2019

ajsutton left a comment

CjHare left a comment

	protected BigInteger chainId = BigInteger.valueOf(-1);
	protected Optional<BigInteger> chainId = Optional.empty();

Allow use of large chain ids #1289

Allow use of large chain ids #1289

Conversation

jframe commented Apr 16, 2019

PR description

Fixed Issue(s)

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jframe commented Apr 18, 2019

ajsutton left a comment

Choose a reason for hiding this comment

CjHare left a comment

Choose a reason for hiding this comment