PrivacyMarkerTransaction to be signed with a randomly generated key #1844
Conversation
| @Override | ||
| public Transaction create( | ||
| final String transactionEnclaveKey, final PrivateTransaction privateTransaction) { | ||
| final KeyPair keyPair = KeyPair.generate(); |
There was a problem hiding this comment.
Just to note - this can only be used in networks where the minimum gas price is 0
| @@ -182,7 +182,6 @@ | |||
| checkArgument( | |||
| storageProvider == null || rocksDbConfiguration == null, | |||
| "Must supply either storage provider or RocksDB confguration, but not both"); | |||
| privacyParameters.setSigningKeyPair(nodeKeys); | |||
There was a problem hiding this comment.
ATs are failing because the PMT signing key should be set somewhere. From the changes in PrivacyParameters they should be set in the builder somewhere. The reason for setting the signing key here is that the nodeKeys object was not accessible any earlier when initially implemented.
In JsonRpcMethodsFactory.createPrivateMarkerTransactionFactory() you will always be returning a RandomSigningPrivateMarkerTransactionFactory which will sign the PMT with a key that represents an account that has no funds in dev.json genesis file used in the AT
There was a problem hiding this comment.
Thanks for the help, that would have been hard to track down.
| @@ -1185,7 +1185,8 @@ private void synchronize( | |||
| final WebSocketConfiguration webSocketConfiguration, | |||
| final MetricsConfiguration metricsConfiguration, | |||
| final Optional<PermissioningConfiguration> permissioningConfiguration, | |||
| final Collection<EnodeURL> staticNodes) { | |||
| final Collection<EnodeURL> staticNodes) | |||
| throws IOException { | |||
There was a problem hiding this comment.
I can't immediately see why this would now throw IOException. I assume there's a chain of calls it now flows up that's not obvious in GitHub's UI but could you double check it is required with the final code?
There was a problem hiding this comment.
Removed - was hacked in, but resolved more suitably.
rain-on
changed the title
| @@ -92,6 +92,8 @@ public void startNode(final PantheonNode node) { | |||
| params.add(node.getPrivacyParameters().getEnclavePublicKeyFile().getAbsolutePath()); | |||
| params.add("--privacy-precompiled-address"); | |||
| params.add(String.valueOf(node.getPrivacyParameters().getPrivacyAddress())); | |||
| params.add("--privacy-marker-transaction-signing-key-file"); | |||
There was a problem hiding this comment.
I prefer a flag --random-marker-signing (or similar) which is a boolean that defaults to false. If it is false then the marker transaction signing key defaults to --node-private-key-file i.e. uses the fixed transaction signing strategy.
Otherwise, use the random transaction signing strategy.
There was a problem hiding this comment.
That being said - this is also a viable approach. The downside is that in most cases a user needs to set --node-private-key-file and --privacy-marker-transaction-signing-key-file to the same value.
There was a problem hiding this comment.
The problem I see is when we go to mainnet, the account with funds to sign transactions may not be the same key as that used by the node.
Having said that - I suspect this needs to change to be "--signer-url", and Pantheon will need to submit the marker transaction to EthSigner to do the key-management stuff... Not sure how that plays with Acceptance Tests :/
...theon/ethereum/privacy/markertransaction/FixedKeySigningPrivateMarkerTransactionFactory.java
Show resolved
Hide resolved
|
|
||
| public class LatestNonceProvider implements NonceProvider { | ||
|
|
||
| final BlockchainQueries blockchain; |
There was a problem hiding this comment.
These fields can be private
| this.privacyPrecompileAddress = privacyPrecompileAddress; | ||
| } | ||
|
|
||
| protected Address getPrivacyPrecompileAddress() { |
| public Transaction create( | ||
| final String transactionEnclaveKey, final PrivateTransaction privateTransaction) { | ||
| final KeyPair signingKey = KeyPair.generate(); | ||
| return create(transactionEnclaveKey, privateTransaction, 0, signingKey); |
There was a problem hiding this comment.
This could possibly fail randomly though quite unlikely. Should we try again if we fails? Perhaps it's so unlikely not even worth considering?
There was a problem hiding this comment.
Went over this with AJS - we could - but Ethereum is based on the premise that we won't have key collisions.
If we wanted to go down that route, we could extract the nonce from the worldstate then if not 0 - try another generation ... and then? Repeat until successful?
There was a problem hiding this comment.
True it all starts to fall apart if we are wondering about key collisions. Don't think we need to do anything then.
| } | ||
|
|
||
| @Test | ||
| public void nonceInsertedToTransactionIsThatProvidedFromNonceProvider() { |
There was a problem hiding this comment.
This test seems like it's doing more than just checking that inserted nonce is provided nonce. Perhaps something like createsPrivateMarkerTransactionUsingProvidedNonce or something like that?
There was a problem hiding this comment.
yup - better. Done.
|
|
||
| private PrivateMarkerTransactionFactory createPrivateMarkerTransactionFactory( | ||
| final PrivacyParameters privacyParameters, | ||
| final BlockchainQueries blockchain, |
There was a problem hiding this comment.
Variable name is bit misleading, blockchainQueries?
This udpates the PrivateTransactionHandler to sign privacy marker transactions with a randomly generated keypair (rather than the node key).
The code has been architectured such that moving forwards the user can either specify a keyfile on the command line (and pass into PrivacyParameters.Builder) - or the system will otherwise perform random-key signing.