Add `FullNode` support #21

alekseysidorov · 2017-04-11T12:45:07Z

Closes #20

dj8yfo · 2017-04-27T13:23:23Z

src/handler/auditing.rs

+        // Verify tx content
+        self.check_anchoring_tx_content(&tx, cfg, state)?;
+        // Checks with access to the `bitcoind`
+        if let Some(ref client) = self.client {


@slowli
What use of verifying anchoring without bitcoind? Consistency of inner exonum transactions? Does it really matter without comparison against bitcoin transactions?.

Auditing nodes should probably verify all Exonum transactions; this is a reasonable default. In the case of anchoring, right now internal Exonum verifications without bitcoind don't matter much, but they could become much more useful if we introduce a Bitcoin Blockchain oracle.

@slowli @alekseysidorov I assume verification of inner consistency should be done during Transaction::execute(). If the transaction doesn't satisfy necessary restrictions, it should be discarded without changing state_hash. If state_hash of node's block differs from majority's' state_hash, it'll get a panic.

That will works fine if during the MsgAnchoringLect execution we ensure that at least 2/3+1 known prev_hash as lect.
After when this check will be implemented auditor_node will check anchoring consistency automaticaly during blocks execution.

dj8yfo · 2017-04-27T13:37:27Z

src/handler/auditing.rs

+
+            // Get previous tx
+            let prev_txid = tx.prev_hash().be_hex_string();
+            let prev_tx = if let Some(tx) = client.get_transaction(&prev_txid)? {


@slowli
Is there any verification that tx is the only valid output of prev_tx, used for anchoring? (if tx has 2+ outputs, it can still support anchoring different forks).

Also, i don't see, how the whole lect chain may unwound by check_anchoring_lect: starting from trusted FundingTx.

@alekseysidorov please, clarify.

dj8yfo · 2017-04-27T14:06:45Z

src/handler/auditing.rs

+        let anchoring_schema = AnchoringSchema::new(state.view());
+        // Check that tx address is correct
+        let tx_addr = tx.output_address(cfg.network);
+        let addr = if let Some(following) = anchoring_schema.following_anchoring_config()? {


@alekseysidorov is this is the place to replace with anchoring_schema.following_anchoring_config_at_height(state.height())
?

dj8yfo · 2017-04-27T14:08:58Z

src/handler/auditing.rs

+                }
+                TxKind::Other(tx) => {
+                    let e = HandlerError::IncorrectLect {
+                        reason: "Weird input transaction".to_string(),


Change to a more descriptive message, please, like Transaction format not corresponding to expected: TxKind::Anchoring or TxKind::FundingTx

dj8yfo · 2017-04-27T14:42:04Z

src/handler/auditing.rs

+
+                if let Some((lect, count)) = lects.iter().max_by_key(|&(_, v)| v) {
+                    if *count >= ::majority_count(validators_count as u8) {
+                        match TxKind::from(lect.clone()) {


maybe a helper function for matching -> Result<LectKing, Error> ?
there's one more occurence

dj8yfo · 2017-04-27T14:46:04Z

src/service.rs

+                error!("An error occured: {}", e);
+                handler.errors.push(e);
+                Ok(())
+            }
            Err(e) => {


@alekseysidorov @slowli propose doing panic for all of these, when 'cfg(not(feature="sandbox_tests")'.
Looking at all occurencies of

pub enum Error { + IncorrectLect { reason: String, tx: BitcoinTx },

all of them are unrecoverable error in anchoring verification. It would be harder to spot such during benchmark if it only goes to log (they shouldn't occur during normal operation, only in some byzantine/fork/[validators' collusion] scenarios).
Also it will allow to spot verification logic errors with respect to independent bitcoind instances for each of exonum nodes, which may see bitcoin ledger differently at the same point of time. After corresponding benchmark (separate [validator+bitcoind], separate [fullnode+bitcoind]), additional adjustments for that inconsistency may have to be made.
Not so sure about `LectNotFound' though.

dj8yfo

panics around IncorrectLect, and, very likely, LectNotFound

slowli

Surface-level review as of 5c5039c. Mostly nitpicks; sorry for that. I'll try to review the code one more time soon, focusing on logic.

slowli · 2017-05-04T16:20:56Z

sandbox_tests/tests/anchoring.rs

@@ -651,14 +651,52 @@ fn test_anchoring_lect_incorrect_anchoring_payload() {
    assert_eq!(lects_before, lects_after);
 }

+// We received correct lect with the unknown prev_hash
+// problems: None
+// result: we ignores it


Nitpick: "ignores" -> "ignore".

slowli · 2017-05-04T16:21:52Z

sandbox_tests/src/lib.rs

@@ -64,6 +65,12 @@ impl AnchoringSandboxState {
        self.handler.lock().unwrap()
    }

+    pub fn take_errors(&self) -> Vec<HandlerError> {
+        let mut handler = self.handler();
+        let v = handler.errors.drain(..).collect::<Vec<_>>();


Wouldn't it be possible to just return handler.errors... without declaring v?

No, I want to take values instead of clonning.

slowli · 2017-05-04T16:38:26Z

sandbox_tests/tests/auditing.rs

+use anchoring_btc_sandbox::{AnchoringSandboxState, initialize_anchoring_sandbox};
+use anchoring_btc_sandbox::helpers::*;
+
+fn gen_following_cfg(sandbox: &Sandbox,


I'd add a comment with a description of how the following config differs from the previous one.

slowli · 2017-05-04T16:45:01Z

sandbox_tests/tests/auditing.rs

+    client.expect(vec![request! {
+            method: "getrawtransaction",
+            params: [&anchored_tx.txid(), 1],
+            response: {


Would it be possible to collapse 3 instances of this transaction in this file into one?

slowli · 2017-05-04T16:46:37Z

sandbox_tests/tests/auditing.rs

+        }]);
+    add_one_height_with_transactions(&sandbox, &sandbox_state, &[cfg_tx]);
+
+    // Check enough confirmations case


Maybe, split this into two tests? Or is the previous block not supposed to check anything? 🤔 In the latter case, the comment should be reworded in order not to give a wrong impression.

slowli · 2017-05-04T17:26:23Z

src/blockchain/transactions.rs

+                                     id));
+
+                if prev_lect.id() != prev_txid {
+                    panic!("Inconsistency in blockchain found!");


Should probably be more specific here, like "Inconsistent reference to previous lect in Exonum and Bitcoin" (if I understand this check correctly), maybe with further details.

slowli · 2017-05-04T17:44:53Z

src/handler/auditing.rs

+        }
+        if tx.find_out(&addr).is_some() {
+            let e = HandlerError::IncorrectLect {
+                reason: "Initial funding_tx have wrong output address".to_string(),


Nitpick: "have" -> "has".

slowli · 2017-05-04T17:49:55Z

src/handler/basic.rs

+    pub fn client(&self) -> &AnchoringRpc {
+        self.client
+            .as_ref()
+            .expect("Client need to be exists for validator")


Nitpick: I'd say "Bitcoin client needs to be present for validator node".

slowli · 2017-05-04T17:56:36Z

src/handler/basic.rs

+            Ok(AnchoringState::Auditing { cfg: actual })
+        } else {
+            let state = if let Some(cfg) = self.following_config(state)? {
+                if actual.redeem_script().1 != cfg.config.redeem_script().1 {


Nitpick: cfg.config is hurting my brain. Maybe, rename cfg -> following?

I replace it in #38.

slowli · 2017-05-04T18:10:46Z

src/handler/error.rs

+        }
+    }
+
+    fn cause(&self) -> Option<&::std::error::Error> {


Isn't ::std:: qualifier unnecessary here? You used error::Error previously.

slowli

Looks mergeable as of bea916d.

dj8yfo · 2017-05-10T22:28:52Z

src/blockchain/schema.rs

@@ -124,8 +130,7 @@ impl<'a> AnchoringSchema<'a> {
    }


The method pub fn lect(&self, validator: u32) -> Result<BitcoinTx, StorageError> contains an implicit unwrap. Does the assumption, that corresponding lects table contains at least one element, always holds true when the method is called? It has many usages.

Yes we assume that at least initial funding transaction exist in lect table. This tx is root of anchoring chain.

dj8yfo · 2017-05-10T22:44:46Z

src/handler/error.rs

+impl error::Error for Error {
+    fn description(&self) -> &str {
+        match *self {
+            Error::IncorrectLect { .. } => "Incorrect lect",


format!("Incorrect lect: {}, tx={:#?}", reason, tx) ?

And are how you going to return reference to local owned value.

dj8yfo · 2017-05-11T15:58:29Z

src/service.rs

+                if let HandlerError::IncorrectLect { .. } = e {
+                    panic!("A critical error occured: {}", e);
+                }
+                error!("An error occured: {}", e);


@alekseysidorov Maybe we should panic, if HandlerError::LectNotFound didn't occur for too long?

By collecting the count of failed attempts in NodeState. If it were problems with bitcoind, fix them and then recover after node restart (count equal to 0 again). Just an idea.

It seems to looking like crutch. We needs to investigate a normal events mechanism to inform the node admin about errors.

dj8yfo · 2017-05-11T16:36:51Z

src/handler/auditing.rs

+        Ok(())
+    }
+
+    fn check_anchoring_lect(&self, tx: AnchoringTx) -> Result<(), ServiceError> {


This check doesn't try to compare against tx.payload().height, which may have occured too long ago (cfg.latest_anchoring_height() - 5000 ). I assume, with this code anchoring audit will pass for fullnode, if there's at least one lect existing in bitcoin blockchain which is last for majority of validators.

@alekseysidorov are you handling this in another pr?

I think it could be handled in another PR.

If I understand correctly, atm check_funding_lect and check_anchoring_lect only verify that the supplied transaction is present in the Bitcoin network (if I interpret get_transaction correctly, the transaction does not even need to be confirmed); this may be too lax. The validators may just stall the anchoring process by not coming up with new lects, which can be discovered with the check you describe. Maybe, the number of confirmations on the Bitcoin blockchain should be checked as well, although I cannot yet come up with a stalling/malicious strategy that would be foiled by this check.

Another interesting question is: can the present check trigger a false alarm, e.g., if an anchoring transaction had been orphaned due to blockchain reorg? That would be a more pressing issue, imo.

@slowli. Would it be enough to wait for 3 confirmations? To cover some 99,999% of situations, when bitcoin forks/reorganizes. For the remaining 0,0001% it still won't help.

I assume, the possibility of fork/reorganization has to be handled anyway, as it would be the same problem for bitcoin relay approach. @VukW mentioned something similar.

@gisochre I'm sorry; the problematic scenario I had in mind does not actually involve a blockchain reorg. It goes like this:

Validators come up with a lect and send it to the Bitcoin network

The transaction in the lect is somehow mutated (say, by a malicious validator)

The mutated transaction is eventually mined

Thus, the original lect transaction is orphaned

Some time later (say, 1 year) an auditor starts syncing with the Exonum blockchain and encounters the collected lect that is not present in the Bitcoin blockchain (and which has never appeared in any Bitcoin block)

It seems that the auditor will panic in this case because get_transaction RPC will return None; bitcoind obviously does not persist double-spent txs that have never been in a block.

Now, a similar situation could occur during a reorg, when an anchoring transaction becomes unconfirmed. If the tx was double-spent in this case, it could potentially lead to the same problem. I'm not sure, but it seems logical that bitcoind would not download orphaned blocks from far past, so get_transaction might return None for the collected lect for a syncing node (interestingly: would it be deterministic, or depend on how far in the past the reorg happened?). But, as far as I understand, the validators would not actually abandon/double-spend the newly unconfirmed tx after the reorg; they would rather continue building new anchoring txs on top of it. Thus, the reorg scenario is not really a problem.

Maybe, I'm missing something completely? For instance, I don't quite get what you mean by waiting for 3 confirms: 3 confirms between adjacent anchorings?

The transaction in the lect is somehow mutated (say, by a malicious validator)

How? signature malleability has been fixed, i believe.

Thus, the reorg scenario is not really a problem.

then no need in 3 confirmations i guess. 3 confirmations before a validator updates his last lect (it may have been very hard/impossible to fit into current implementation).

How? signature malleability has been fixed, i believe.

The signature malleability by the signature author cannot be fixed by anything short of SegWit; it's always possible for the author to create another valid signature over the same message (only in SegWit it would not invalidate the following anchoring txs).

Additionally, there are marginal cases, in which tx signatures may be malleated by third parties.

alekseysidorov requested a review from dj8yfo April 27, 2017 10:08

dj8yfo reviewed Apr 27, 2017

View reviewed changes

dj8yfo approved these changes Apr 27, 2017

View reviewed changes

dj8yfo suggested changes Apr 27, 2017

View reviewed changes

stanislav-tkach mentioned this pull request Apr 28, 2017

Refactor error handling exonum/exonum#57

Open

dj8yfo mentioned this pull request Apr 28, 2017

Proposal to implement stand-alone procedure of anchoring verification. #37

Closed

Aleksei Sidorov added 19 commits May 3, 2017 17:32

Simplify service constructor

6af30da

Fix typo

b9bdcaf

Auditor support's draft

c4d2d99

Use configuration revision with fullnode support

7ab39be

Initial working prototype

99c2dec

Progress in auditing test

e5c73f7

Implement sandbox for handler errors

3777507

Add simple auditing test

cfe706d

Add auditing tests

af63565

Implement bitcoind based test for funding_tx

e6d8551

Implement first bitcoind based test for anchoring

074f967

Implement some test for lect inputs

e744f0a

Remove Cargo.lock

cd0ba90

Update exonum rev

e7c41fe

Format code

85f453a

Implement test for prev_lect checks

a1145ae

Show height in LectNotFound error

749e261

Relax checks in the auditing mode.

2de332a

Fix after rebase

5c5039c

alekseysidorov changed the title ~~WIP: Add FullNode support~~ Add FullNode support May 3, 2017

slowli reviewed May 4, 2017

View reviewed changes

Aleksei Sidorov added 5 commits May 5, 2017 17:29

Fix test for funding_tx

3c95125

Resolve nitpicks

94afc86

Reduce boiler plate code

c1f160f

Resole some more nitpicks

bcac1c9

Use human readable constant

4fb36f2

alekseysidorov requested review from vldm, slowli and dj8yfo May 6, 2017 16:51

Aleksei Sidorov added 2 commits May 9, 2017 14:54

Introduce fast_forward helper for sandbox

d8ffeaa

Fix errors handler in service

27fe4f6

stanislav-tkach approved these changes May 9, 2017

View reviewed changes

Aleksei Sidorov added 6 commits May 10, 2017 12:40

Update rustfmt version

f367175

Remove hardcoded constant

9ad62a5

Fix rustfmt version

c3b6531

Refactor collect_lects

e90ee9a

Rename method to latest_anchored_height

ecfd3a0

Fix table prefixes in schema

bea916d

slowli approved these changes May 11, 2017

View reviewed changes

Make log output less verbose

6df3a6b

dj8yfo approved these changes May 11, 2017

View reviewed changes

alekseysidorov merged commit 4f558ca into exonum:master May 12, 2017

alekseysidorov deleted the 20-fullnode-support branch June 1, 2017 09:18

Add FullNode support #21

Add FullNode support #21

Conversation

alekseysidorov commented Apr 11, 2017

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

dj8yfo Apr 27, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dj8yfo left a comment

Choose a reason for hiding this comment

slowli 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

slowli 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

dj8yfo May 11, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Add `FullNode` support #21

Add `FullNode` support #21

dj8yfo Apr 27, 2017 •

edited

dj8yfo May 11, 2017 •

edited