Merge #1315

1315: Problem: revised fees not implemented (fixes #1302) r=tomtau a=tomtau

Solution: deposit, transfer, withdraw enforce min fee;
(unbond tx already only had min fee)
adjusted tests


Co-authored-by: Tomas Tauber <[email protected]>

chain-abci/src/enclave_bridge/real/server.rs

@@ -80,8 +80,8 @@ impl TxValidationServer {
                                     let min_fee = last_state
                                         .top_level
                                         .network_params
-                                        .get_min_const_fee()
-                                        .expect("invalid fee policy");
+                                        .calculate_fee(req.tx_size as usize)
+                                        .expect("valid fee");
                                     let info = ChainInfo {
                                         min_fee_computed: min_fee,
                                         chain_hex_id: self.network_id,

chain-abci/src/enclave_bridge/real/test/seal.rs

@@ -186,12 +186,11 @@ pub fn test_sealing() {
         }
     };
 
-    let halfcoin = Coin::from(5000_0000u32);
     let utxo1 = TxoPointer::new(*txid, 0);
     let mut tx1 = Tx::new();
     tx1.attributes = TxAttributes::new(TEST_NETWORK_ID);
     tx1.add_input(utxo1.clone());
-    tx1.add_output(TxOut::new(eaddr.clone(), halfcoin));
+    tx1.add_output(TxOut::new(eaddr.clone(), Coin::one()));
     let txid1 = tx1.id();
     let witness1 = vec![TxInWitness::TreeSig(
         schnorr_sign(&secp, &Message::from_slice(&txid1).unwrap(), &secret_key),

chain-abci/tests/abci_app.rs

@@ -432,12 +432,17 @@ fn prepare_app_valid_tx() -> (ChainNodeApp<MockClient>, TxAux, WithdrawUnbondedT
     let public_key = PublicKey::from_secret_key(&secp, &secret_key);
     let addr = RedeemAddress::from(&public_key);
     let app = init_chain_for(addr);
-
     let tx = WithdrawUnbondedTx::new(
         0,
         vec![
             TxOut::new_with_timelock(ExtendedAddr::OrTree([0; 32]), Coin::one(), 0),
             TxOut::new_with_timelock(ExtendedAddr::OrTree([1; 32]), Coin::unit(), 0),
+            // leftover -- in previous tests, it was all paid as a fee
+            TxOut::new_with_timelock(
+                ExtendedAddr::OrTree([2; 32]),
+                Coin::new(9999999999899999668).unwrap(),
+                0,
+            ),
         ],
         TxAttributes::new_with_access(0, vec![TxAccessPolicy::new(public_key, TxAccess::AllData)]),
     );
@@ -572,9 +577,9 @@ fn deliver_tx_should_add_valid_tx() {
     assert_eq!(1, app.delivered_txs.len());
     assert_eq!(1, cresp.events.len());
     assert_eq!(3, cresp.events[0].attributes.len());
-    // the unit test transaction just has two outputs: 1 CRO + 1 carson / base unit, the rest goes to a fee
+    // the unit test transaction just three outputs: 1 CRO + 1 carson / base unit + the rest
     assert_eq!(
-        &b"99999999998.99999998".to_vec(),
+        &b"0.00000330".to_vec(),
         &cresp.events[0].attributes[0].value
     );
     assert_eq!(
@@ -814,6 +819,7 @@ fn all_valid_tx_types_should_commit() {
             TxOut::new_with_timelock(eaddr.clone(), Coin::one(), 0),
             TxOut::new_with_timelock(eaddr.clone(), (Coin::one() + Coin::one()).unwrap(), 0),
             TxOut::new_with_timelock(eaddr.clone(), Coin::one(), 0),
+            TxOut::new_with_timelock(eaddr.clone(), Coin::new(9999999999599999619).unwrap(), 0), // rest
         ],
         TxAttributes::new_with_access(0, vec![TxAccessPolicy::new(public_key, TxAccess::AllData)]),
     );
@@ -843,11 +849,10 @@ fn all_valid_tx_types_should_commit() {
         let spend_utxos = get_tx_meta(&txid, &app);
         assert!(!spend_utxos.any());
     }
-    let halfcoin = Coin::from(5000_0000u32);
     let utxo1 = TxoPointer::new(*txid, 0);
     let mut tx1 = Tx::new();
     tx1.add_input(utxo1);
-    tx1.add_output(TxOut::new(eaddr, halfcoin));
+    tx1.add_output(TxOut::new(eaddr, Coin::from(99999685u32)));
     let txid1 = tx1.id();
     let witness1 = vec![TxInWitness::TreeSig(
         schnorr_sign(&secp, &Message::from_slice(&txid1).unwrap(), &secret_key),
@@ -1018,7 +1023,7 @@ fn query_should_return_proof_for_committed_tx() {
 
     app.begin_block(&env.req_begin_block(1, 0));
 
-    let tx_aux = env.unbond_tx((Coin::max() / 10).unwrap(), 0, 0);
+    let tx_aux = env.unbond_tx(Coin::new(5000000000000000000).unwrap(), 0, 0);
     let rsp_tx = app.deliver_tx(&RequestDeliverTx {
         tx: tx_aux.encode(),
         ..Default::default()

chain-abci/tests/tx_validation.rs

@@ -229,7 +229,7 @@ fn prepare_app_valid_transfer_tx(
     tx.add_output(TxOut::new(addr, Coin::new(9).unwrap()));
     let sk2 = SecretKey::from_slice(&[0x11; 32]).expect("32 bytes, within curve order");
     let addr2 = get_address(&secp, &sk2).0;
-    tx.add_output(TxOut::new(addr2, Coin::new(1).unwrap()));
+    tx.add_output(TxOut::new(addr2, Coin::new(99999634).unwrap()));
 
     let witness: Vec<TxInWitness> = vec![get_tx_witness(secp, &tx.id(), &secret_key, &merkle_tree)];
     let plain_txaux = PlainTxAux::new(tx.clone(), witness.clone().into());
@@ -428,7 +428,7 @@ fn prepare_app_valid_withdraw_tx(
 
     let outputs = vec![
         TxOut::new_with_timelock(addr1, Coin::new(9).unwrap(), 0),
-        TxOut::new_with_timelock(addr2, Coin::new(1).unwrap(), 0),
+        TxOut::new_with_timelock(addr2, Coin::new(99999745).unwrap(), 0),
     ];
 
     let tx = WithdrawUnbondedTx::new(1, outputs, TxAttributes::new(DEFAULT_CHAIN_ID));

chain-tx-enclave/tx-query/enclave/src/lib.rs

@@ -159,38 +159,44 @@ fn get_sealed_request(req: &EncryptionRequest, txid: &TxId) -> Option<Vec<u8>> {
     Some(sealed_log)
 }
 
-fn construct_request(req: &EncryptionRequest) -> Option<QueryEncryptRequest> {
-    let (txid, sealed, tx_inputs) = match req {
+fn construct_request(req: &EncryptionRequest, req_len: usize) -> Option<QueryEncryptRequest> {
+    let (txid, sealed, tx_inputs, tx_size) = match req {
+        // TODO: are the size estimates ok?
         EncryptionRequest::TransferTx(tx, _) => {
             let txid = tx.id();
             let sealed = get_sealed_request(&req, &txid);
             let tx_inputs = Some(tx.inputs.clone());
-            (txid, sealed, tx_inputs)
+            (txid, sealed, tx_inputs, req_len + 34 * tx.inputs.len() + 74)
         }
         EncryptionRequest::DepositStake(tx, _) => {
             let txid = tx.id();
             let sealed = get_sealed_request(&req, &txid);
             let tx_inputs = Some(tx.inputs.clone());
-            (txid, sealed, tx_inputs)
+            (txid, sealed, tx_inputs, req_len + 34 * tx.inputs.len() + 74)
         }
-        EncryptionRequest::WithdrawStake(tx, _, _) => {
+        EncryptionRequest::WithdrawStake(tx, state, _) => {
             let txid = tx.id();
             let sealed = get_sealed_request(&req, &txid);
-            (txid, sealed, None)
+            let state_len = state.encode().len();
+            // FIXME: no need to send state in request
+            (txid, sealed, None, req_len - state_len + 73)
         }
     };
     sealed.map(|sealed_enc_request| QueryEncryptRequest {
         txid,
         sealed_enc_request,
         tx_inputs,
+        // TODO: checks, but this should fit, as all things are bounded more like u16::max
+        tx_size: tx_size as u32,
     })
 }
 
 fn handle_encryption_request(
     tls: &mut rustls::Stream<rustls::ServerSession, TcpStream>,
     req: EncryptionRequest,
+    req_len: usize,
 ) -> sgx_status_t {
-    let request = construct_request(&req);
+    let request = construct_request(&req, req_len);
     match request {
         None => {
             let _ = tls.sock.shutdown(Shutdown::Both);
@@ -281,7 +287,7 @@ pub extern "C" fn run_server(socket_fd: c_int, timeout: c_int) -> sgx_status_t {
     let mut plain = vec![0; ENCRYPTION_REQUEST_SIZE];
     match tls.read(&mut plain) {
         Ok(l) => match TxQueryInitRequest::decode(&mut &plain.as_slice()[0..l]) {
-            Ok(TxQueryInitRequest::Encrypt(req)) => handle_encryption_request(&mut tls, *req),
+            Ok(TxQueryInitRequest::Encrypt(req)) => handle_encryption_request(&mut tls, *req, l),
             Ok(TxQueryInitRequest::DecryptChallenge) => handle_decryption_request(&mut tls, plain),
             _ => {
                 let _ = conn.shutdown(Shutdown::Both);

chain-tx-validation/src/lib.rs

@@ -258,17 +258,16 @@ fn check_input_output_sums(
     outcoins: Coin,
     extra_info: &ChainInfo,
 ) -> Result<Fee, Error> {
-    // check sum(input amounts) >= sum(output amounts) + minimum fee
+    // check sum(input amounts) == sum(output amounts) + minimum fee
     let min_fee: Coin = extra_info.min_fee_computed.to_coin();
     let total_outsum = outcoins + min_fee;
     if let Err(_coin_err) = total_outsum {
         return Err(Error::InvalidSum); // FIXME: Err(Error::InvalidSum(coin_err));
     }
-    if incoins < total_outsum.unwrap() {
+    if incoins != total_outsum.unwrap() {
         return Err(Error::InputOutputDoNotMatch);
     }
-    let fee_paid = (incoins - outcoins).unwrap();
-    Ok(Fee::new(fee_paid))
+    Ok(Fee::new(min_fee))
 }
 
 /// checks TransferTx -- TODO: this will be moved to an enclave
@@ -383,7 +382,15 @@ pub fn verify_unbonding(
     if maintx.value == Coin::zero() {
         return Err(Error::ZeroCoin);
     }
-    check_input_output_sums(account.bonded, maintx.value, extra_info)?;
+    // check that it's enough for fee + amount to be unbonded
+    let min_fee: Coin = extra_info.min_fee_computed.to_coin();
+    let total_outsum = maintx.value + min_fee;
+    if let Err(_coin_err) = total_outsum {
+        return Err(Error::InvalidSum);
+    }
+    if account.bonded < total_outsum.unwrap() {
+        return Err(Error::InputOutputDoNotMatch);
+    }
     account.unbond(
         maintx.value,
         extra_info.min_fee_computed.to_coin(),

client-core/src/signer/dummy_signer.rs

@@ -1,3 +1,4 @@
+use crate::transaction_builder::WitnessedUTxO;
 use chain_core::common::MerkleTree;
 use chain_core::common::H256;
 use chain_core::init::address::RedeemAddress;
@@ -50,14 +51,13 @@ impl DummySigner {
     }
 
     /// Schnorr sign consecutive imaginary inputs of provided length
-    pub fn schnorr_sign_inputs_len(
-        &self,
-        total_pubkeys_len: usize,
-        inputs_len: usize,
-    ) -> Result<TxWitness> {
-        let dummy_witness = self.sign_tx(total_pubkeys_len)?;
-        Ok(std::iter::repeat(dummy_witness)
-            .take(inputs_len)
+    pub fn schnorr_sign_inputs_len(&self, inputs: &[WitnessedUTxO]) -> Result<TxWitness> {
+        Ok(inputs
+            .iter()
+            .map(|x| {
+                self.sign_tx(x.threshold as usize)
+                    .expect("would that ever fail? why this dummy has results?")
+            })
             .collect::<Vec<TxInWitness>>()
             .into())
     }
@@ -81,9 +81,8 @@ impl DummySigner {
     }
 
     /// Mock the txaux for deposit transactions
-    pub fn mock_txaux_for_deposit(&self, input_len: usize) -> Result<TxAux> {
-        let total_pubkeys_len = 1;
-        let witness = self.schnorr_sign_inputs_len(total_pubkeys_len, input_len)?;
+    pub fn mock_txaux_for_deposit(&self, inputs: &[WitnessedUTxO]) -> Result<TxAux> {
+        let witness = self.schnorr_sign_inputs_len(inputs)?;
         let plain_payload = PlainTxAux::DepositStakeTx(witness);
         let padded_payload = self.pad_payload(plain_payload);
         let deposit_bond_tx = DepositBondTx {

client-core/src/transaction_builder/default_wallet_transaction_builder.rs

@@ -42,26 +42,31 @@ where
     transaction_obfuscation: O,
 }
 
-impl<S, F, O> WalletTransactionBuilder for DefaultWalletTransactionBuilder<S, F, O>
+impl<F, S, O> DefaultWalletTransactionBuilder<S, F, O>
 where
     S: Storage,
     F: FeeAlgorithm + Clone,
     O: TransactionObfuscation,
 {
-    fn build_transfer_tx(
+    /// FIXME: temporary for broken fee estimation
+    #[allow(clippy::too_many_arguments)]
+    fn build_transfer_tx_ex(
         &self,
         name: &str,
         enckey: &SecKey,
         unspent_transactions: UnspentTransactions,
         outputs: Vec<TxOut>,
         return_address: ExtendedAddr,
         attributes: TxAttributes,
+        // FIXME: this should be per unspent_transactions
+        threshold: u16,
     ) -> Result<(TxAux, Vec<TxoPointer>, Coin)> {
         let mut raw_builder = self.select_and_build(
             &unspent_transactions,
             outputs,
             return_address.clone(),
             attributes,
+            threshold,
         )?;
 
         let selected_inputs: Vec<TxoPointer> = raw_builder
@@ -84,6 +89,33 @@ where
 
         Ok((tx_aux, selected_inputs, return_amount))
     }
+}
+
+impl<S, F, O> WalletTransactionBuilder for DefaultWalletTransactionBuilder<S, F, O>
+where
+    S: Storage,
+    F: FeeAlgorithm + Clone,
+    O: TransactionObfuscation,
+{
+    fn build_transfer_tx(
+        &self,
+        name: &str,
+        enckey: &SecKey,
+        unspent_transactions: UnspentTransactions,
+        outputs: Vec<TxOut>,
+        return_address: ExtendedAddr,
+        attributes: TxAttributes,
+    ) -> Result<(TxAux, Vec<TxoPointer>, Coin)> {
+        self.build_transfer_tx_ex(
+            name,
+            enckey,
+            unspent_transactions,
+            outputs,
+            return_address,
+            attributes,
+            1,
+        )
+    }
 
     #[inline]
     fn obfuscate(&self, signed_transaction: SignedTransaction) -> Result<TxAux> {
@@ -130,6 +162,8 @@ where
         outputs: Vec<TxOut>,
         return_address: ExtendedAddr,
         attributes: TxAttributes,
+        // FIXME: this should be per UnspentTransactions
+        threshold: u16,
     ) -> Result<RawTransferTransactionBuilder<F>> {
         let output_value = sum_coins(outputs.iter().map(|output| output.value)).chain(|| {
             (
@@ -152,6 +186,7 @@ where
                 return_address.clone(),
                 change_amount,
                 attributes.clone(),
+                threshold,
             );
 
             let new_fees = raw_tx_builder.estimate_fee()?;
@@ -172,11 +207,13 @@ where
         return_address: ExtendedAddr,
         change_amount: Coin,
         attributes: TxAttributes,
+        // FIXME: this should be per SelectedUnspentTransactions
+        threshold: u16,
     ) -> RawTransferTransactionBuilder<F> {
         let mut raw_tx_builder =
             RawTransferTransactionBuilder::new(attributes, self.fee_algorithm.clone());
         for input in selected_unspent_transactions.iter() {
-            raw_tx_builder.add_input(input.clone());
+            raw_tx_builder.add_input(input.clone(), threshold);
         }
         for output in outputs.iter() {
             raw_tx_builder.add_output(output.clone());
@@ -313,13 +350,14 @@ mod default_wallet_transaction_builder_tests {
         )];
         let attributes = TxAttributes::new(171);
         let (tx_aux, _selected_inputs, _return_amount) = transaction_builder
-            .build_transfer_tx(
+            .build_transfer_tx_ex(
                 name,
                 &enckey,
                 unspent_transactions.clone(),
                 outputs,
                 return_address,
                 attributes,
+                2,
             )
             .unwrap();
 
@@ -351,7 +389,12 @@ mod default_wallet_transaction_builder_tests {
                         }
                     }))
                     .unwrap();
-                    assert!((output_value + fee).unwrap() <= input_value);
+                    // FIXME: all these unit tests don't account for 16-byte tag in encryption
+                    // in those mock encrypt stuff, while the estimation does :/
+                    assert!(
+                        (output_value + fee).unwrap() <=
+                        (input_value - Coin::from(16u32)).unwrap()
+                    );
 
                     for (i, input) in transaction.inputs.iter().enumerate() {
                         let address = if input.id == [3; 32] {