Feature/add submit bitcoin solution new methods #486

martinmedina · 2018-02-19T20:20:56Z

New methods to support submission of merged mining solution:

submitBitcoinBlockTransactions
submitBitcoinBlockPartialMerkle

Both receive less information than old submitBitcoinBlock method which is still available for backward compatibility reasons.

colltoaction

My comments are mostly style issues, but looks good overall.

Regarding functionality, I would appreciate if someone who knew this better could review the mnr_ methods implementations.

colltoaction · 2018-02-19T20:45:34Z

rskj-core/src/main/java/co/rsk/rpc/Web3RskImpl.java

+ * Full responsibility for processing the request is delegated to MinerServer.
+ *
+ * @author Adrian Eidelman
+ * @author Martin Medina
 */
 public class Web3RskImpl extends Web3Impl {


Please consider extracting a Web3MnrModule like here: #483 (in a separate PR!)

Issue created to tackle this in a near future

colltoaction · 2018-02-19T20:51:50Z

rskj-core/src/main/java/co/rsk/rpc/Web3RskImpl.java

+
+        int rskTagPosition = Collections.lastIndexOfSubList(coinbaseAsByteList, rskTagAsByteList);
+        byte[] blockHashForMergedMiningArray = new byte[Keccak256Helper.Size.S256.getValue()/8];
+        System.arraycopy(coinbaseAsByteArray, rskTagPosition+ RskMiningConstants.RSK_TAG.length, blockHashForMergedMiningArray, 0, blockHashForMergedMiningArray.length);


Use Arrays.copyOf.

Elements needs to be copied into a certain position of an existing array and that can not be done with Arrays.copyOf.

colltoaction · 2018-02-19T20:52:44Z

rskj-core/src/main/java/co/rsk/rpc/Web3RskImpl.java

-
-        co.rsk.bitcoinj.core.NetworkParameters params = co.rsk.bitcoinj.params.RegTestParams.get();
+
+        NetworkParameters params = co.rsk.bitcoinj.params.RegTestParams.get();
        new co.rsk.bitcoinj.core.Context(params);


Is this dangling new co.rsk.bitcoinj.core.Context necessary?

good catch, fixed!

colltoaction · 2018-02-19T20:54:03Z

rskj-core/src/main/java/co/rsk/rpc/Web3RskImpl.java

+    }
+
+    public SubmittedBlockInfo mnr_submitBitcoinSolution(String blockHashHex, String blockHeaderHex, String coinbaseHex, String txnHashesHex) {
+        if (logger.isDebugEnabled()) {


Using if (logger.isDebugEnabled()) is an antipattern when your log statement doesn't do any work.

colltoaction · 2018-02-19T20:55:35Z

rskj-core/src/main/java/co/rsk/rpc/Web3RskImpl.java

+    }
+
+    private SubmittedBlockInfo parseResultAndReturn(SubmitBlockResult result) {
+        if("OK".equals(result.getStatus())) {


How about result.isSuccessful()?

as a method of the SubmitBlockResult object ?

colltoaction · 2018-02-19T20:56:11Z

rskj-core/src/main/java/co/rsk/mine/MinerServer.java

@@ -32,6 +33,11 @@

    boolean isRunning();

+    SubmitBlockResult submitBitcoinSolution(String blockHashForMergedMining,


Indent correctly and remove fully-qualified name.

colltoaction · 2018-02-19T20:58:21Z

rskj-core/src/main/java/co/rsk/mine/MinerServerImpl.java

+            BtcBlock blockWithHeaderOnly,
+            BtcTransaction coinbase,
+            List<String> txHashes) {
+        logger.debug("Received solution with hash {} for merged mining", blockHashForMergedMining);


All your logs are debug, but maybe you should consider using info or trace

colltoaction · 2018-02-19T21:01:06Z

rskj-core/src/main/java/co/rsk/rpc/Web3RskImpl.java

+
+    private List<String> parseTransactionHashes(String txnHashesHex) {
+        List<String> txnHashes = new ArrayList<>();
+        for (int start = 0; start < txnHashesHex.length(); start += 64) {


Please create a constant for 64

Since a last minute optimization this code no longer exists

colltoaction · 2018-02-19T21:08:52Z

rskj-core/src/main/java/co/rsk/mine/MinerServerImpl.java


-    public SubmitBlockResult submitBitcoinBlock(String blockHashForMergedMining, co.rsk.bitcoinj.core.BtcBlock bitcoinMergedMiningBlock, boolean lastTag) {
+    SubmitBlockResult submitBitcoinBlock(String blockHashForMergedMining, BtcBlock bitcoinMergedMiningBlock, boolean lastTag) {


should this be private?

It's okay to keep it package private

donequis · 2018-02-22T21:41:25Z

rskj-core/src/main/java/co/rsk/mine/MinerServerImpl.java

+
+        // bits indicates which nodes are going to be used for building the partial merkle tree
+        // for more information please refer to {@link co.rsk.bitcoinj.core.PartialMerkleTree#buildFromLeaves } method
+        byte[] bits = new byte[(int) Math.ceil(bitList.size() / 8.0)];


(bitList.size() + 7) / 8 with integer arithmetic

good idea! change done.

donequis · 2018-02-22T21:46:00Z

rskj-core/src/main/java/co/rsk/mine/MinerServerImpl.java

-        byte[] bitvector = new byte[(int) Math.ceil(txs.size() / 8.0)];
-        co.rsk.bitcoinj.core.Utils.setBitLE(bitvector, 0);
-        return co.rsk.bitcoinj.core.PartialMerkleTree.buildFromLeaves(bitcoinMergedMiningBlock.getParams(), bitvector, txHashes);
+        byte[] bitvector = new byte[(int) Math.ceil(txHashes.size() / 8.0)];


same as before

… hashes. Data sent over network is now of size log_2(block_txs_hash) vs block_txs_hash.

rskops · 2018-02-28T20:37:46Z

SonarQube analysis reported 40 issues

2 critical
16 major
10 minor
12 info

Top 10 issues

MinerServerImpl.java#L314: Make the enclosing method "static" or remove this set.
MinerServerImpl.java#L318: Make the enclosing method "static" or remove this set.
MinerServerImpl.java#L121: Constructor has 8 parameters, which is greater than 7 authorized.
MinerServerImpl.java#L287: Either log or rethrow this exception.
MinerServerImpl.java#L288: Return an empty array instead of null.
MinerServerImpl.java#L313: Incorrect lazy initialization of static field co.rsk.mine.MinerServerImpl.privKey0 in co.rsk.mine.MinerServerImpl.generateFallbackBlock()
MinerServerImpl.java#L314: java/io/File.(Ljava/lang/String;Ljava/lang/String;)V reads a file whose location might be specified by user input
MinerServerImpl.java#L317: Incorrect lazy initialization of static field co.rsk.mine.MinerServerImpl.privKey1 in co.rsk.mine.MinerServerImpl.generateFallbackBlock()
MinerServerImpl.java#L318: java/io/File.(Ljava/lang/String;Ljava/lang/String;)V reads a file whose location might be specified by user input
MinerUtils.java#L69: Define and throw a dedicated exception instead of using a generic one.

colltoaction reviewed Feb 19, 2018

View reviewed changes

martinmedina force-pushed the feature/add-submit-bitcoin-solution-method branch from cfd5741 to 534100b Compare February 20, 2018 05:59

martinmedina requested a review from juli February 21, 2018 21:55

martinmedina changed the title ~~Feature/add submit bitcoin solution method~~ Feature/add submit bitcoin solution new methods Feb 21, 2018

donequis reviewed Feb 22, 2018

View reviewed changes

donequis previously approved these changes Feb 28, 2018

View reviewed changes

martinmedina added 22 commits February 28, 2018 17:05

Add new submitSolution method to web3 mining interface

c7aa73d

Add submitSolution method (first approach) to MinerServer

2021b01

Split tx hashes and call submitBitcoinSolution method

122d8d0

Refactor mining request handling interface

6327169

Remove unused parameter

b2f5568

Remove duplicate code

d677da0

Duplicated code removed

22ce0d9

Refactor on miner server test class

f1f8c16

Add tests for submitBitcoinSolution method

1a37b12

Add new test case for submitBitcoinSolution method

cd41746

Replace fully qualified with import

b61b305

Apply linter to match new styleguide

fa69f62

Transactions come in space separated format

794379a

Partial merkle tree hashes are received instead of full list with txn…

f201273

… hashes. Data sent over network is now of size log_2(block_txs_hash) vs block_txs_hash.

Replace fully qualified with import

f515c1a

Remove unnecessary if condition for logging

76f2b4d

Add missing parameter to constructor on test

5da315e

Apply linter to match new styleguide

f6cf779

Merkle hashes are reversed

f546ba3

Method to accept mining solution with bitcoin block transactions added

90ccbba

Do calculation with integer arithmetic instead of float

813b526

Change tests to match constructor

ab52e54

martinmedina dismissed donequis’s stale review via ab52e54 February 28, 2018 20:25

martinmedina force-pushed the feature/add-submit-bitcoin-solution-method branch from bbd3de0 to ab52e54 Compare February 28, 2018 20:25

donequis approved these changes Feb 28, 2018

View reviewed changes

aeidelman merged commit 0443acb into master Mar 2, 2018

aeidelman deleted the feature/add-submit-bitcoin-solution-method branch March 2, 2018 11:40

aeidelman added this to the Bamboo v0.4.1 milestone Mar 18, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/add submit bitcoin solution new methods #486

Feature/add submit bitcoin solution new methods #486

martinmedina commented Feb 19, 2018 •

edited

Loading

colltoaction left a comment

colltoaction Feb 19, 2018

martinmedina Feb 20, 2018

colltoaction Feb 19, 2018

martinmedina Feb 20, 2018

colltoaction Feb 19, 2018

martinmedina Feb 20, 2018

colltoaction Feb 19, 2018

martinmedina Feb 20, 2018

colltoaction Feb 19, 2018

martinmedina Feb 20, 2018

colltoaction Feb 19, 2018

martinmedina Feb 20, 2018

colltoaction Feb 19, 2018

colltoaction Feb 19, 2018

martinmedina Feb 20, 2018

colltoaction Feb 19, 2018

martinmedina Feb 20, 2018

donequis Feb 22, 2018

martinmedina Feb 22, 2018

donequis Feb 22, 2018

martinmedina Feb 22, 2018

rskops commented Feb 28, 2018

		@@ -32,6 +33,11 @@

		boolean isRunning();

		SubmitBlockResult submitBitcoinSolution(String blockHashForMergedMining,


		public SubmitBlockResult submitBitcoinBlock(String blockHashForMergedMining, co.rsk.bitcoinj.core.BtcBlock bitcoinMergedMiningBlock, boolean lastTag) {
		SubmitBlockResult submitBitcoinBlock(String blockHashForMergedMining, BtcBlock bitcoinMergedMiningBlock, boolean lastTag) {

Feature/add submit bitcoin solution new methods #486

Feature/add submit bitcoin solution new methods #486

Conversation

martinmedina commented Feb 19, 2018 • edited Loading

colltoaction left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rskops commented Feb 28, 2018

Top 10 issues

martinmedina commented Feb 19, 2018 •

edited

Loading