Errata for UTXO Wallet Assignment

Errata.md

This document contains corrections to mistakes in the UTXO wallet assignmetn for PBA's Hong Kong cohort.

Incorrect comment about input and output value

The doc comments on the transaction type incorrectly state that the output value should exceed the input value. This is exactly backwards. Luckily this is only a comment mistake, the tests are correct and this should not affect your programmin experience at all.

/// A Bonecoin Transaction
///
- /// In order for a bonecoin transaction to be valid, it must consume fewer bones than it creates (or an equal number).
+ /// In order for a bonecoin transaction to be valid, it must consume more bones than it creates (or an equal number).
/// As always, the wallet does not need to check incoming transactions, but it does need to ensure that it is not creating invalid transactions for its users.
#[derive(Clone, Hash, Eq, PartialEq, Debug)]
pub struct Transaction {
    pub inputs: Vec<Input>,
    pub outputs: Vec<Coin>,
}

Tests don't check re-orgs thoroughly enough

My provided tests for forks and re-org are not specific enough. They are still correct and should still pass, but they will miss several errors. The problem is that instead of constructing two different chains, they often accidentally construct the same chain twice. We mitigate this problem by adding a transaction to one side of the fork.

You may fix this any way you want, but here is a technique I recommend. You may replace the provided deep_reorg test withi this code and update the other tests accordingly

/// Creates a simple transaction that is unlikely to conflict with any other
/// transactions in your tests. This is useful to when you are intentionally creating
/// a fork. By including a marker transaction on one side of the fork, but not the other,
/// you make sure that the two chains are truly different.
fn marker_tx() -> Transaction {
    Transaction {
        inputs: vec![Input::dummy()],
        outputs: vec![Coin {
            value: 123,
            owner: Address::Custom(123),
        }],
    }
}

#[test]
fn deep_reorg() {
    // Create node and wallet
    let mut node = MockNode::new();
    let mut wallet = wallet_with_alice();

    // Sync a chain to height 3
    let old_b1_id = node.add_block_as_best(Block::genesis().id(), vec![]);
    let old_b2_id = node.add_block_as_best(old_b1_id, vec![]);
    let _old_b3_id = node.add_block_as_best(old_b2_id, vec![]);
    wallet.sync(&node);

    // Reorg to longer chain of length 5

    // In the original test, b1 was identical to old_b1 (and same for b2 and b3)
    // This means that was wasn't really a fork or re-org at all. I just extended
    // the same old chain. By including a marker transaction on one side of the fork,
    // we make sure the blocks are truly unique. It is not necessary to include the marker
    // in all the descendants. Once we have modified a single block all descendants will
    // also be modified because of the parent pointers. Do this for all of your re-org tests.
    let b1_id = node.add_block(Block::genesis().id(), vec![marker_tx()]);
    let b2_id = node.add_block_as_best(b1_id, vec![]);
    let b3_id = node.add_block_as_best(b2_id, vec![]);
    let b4_id = node.add_block_as_best(b3_id, vec![]);
    let b5_id = node.add_block_as_best(b4_id, vec![]);
    wallet.sync(&node);

    assert_eq!(wallet.best_height(), 5);
    assert_eq!(wallet.best_hash(), b5_id);
}

Ambiguous error variants for creat_automatic_transaction

The description of the create_automatic_transaction function does not clearly specify which errors should be returned in which case. Some students pointed out that there could be an InsufficientFunds error.

I have modified the private tests for this function so that in all error cases you just need to return some error; not a specific variant.