Merge pull request #32 from mempool/mononaut/fix-fee-underflow

fix missing prevout tx fee underflow

.github/workflows/ci.yml

@@ -36,6 +36,23 @@ jobs:
           components: rustfmt
       - run: cargo fmt --all -- --check
 
+  test:
+    name: Run Tests
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v3
+      - id: toolchain
+        uses: dtolnay/[email protected]
+      - name: Cache dependencies
+        uses: actions/cache@v3
+        with: # test cache key is different (adding test cfg is a re-compile)
+          path: |
+            ~/.cargo/registry
+            ~/.cargo/git
+            target
+          key: ${{ runner.os }}-cargo-test-${{ steps.toolchain.outputs.cachekey }}-${{ hashFiles('**/Cargo.lock') }}
+      - run: cargo test --package electrs --lib --all-features
+
   clippy:
     name: Linter
     runs-on: ubuntu-latest
@@ -63,4 +80,4 @@ jobs:
             target
           key: ${{ runner.os }}-cargo-${{ steps.toolchain.outputs.cachekey }}-${{ hashFiles('**/Cargo.lock') }}
       - name: Clippy with Features = ${{ matrix.features }}
-        run: cargo clippy ${{ matrix.features }}
+        run: cargo clippy ${{ matrix.features }} -- -D warnings

.gitignore

@@ -4,3 +4,4 @@ target
 *.sublime*
 *~
 *.pyc
+.vscode

scripts/checks.sh

@@ -56,3 +56,7 @@ cargo clippy $@ -q -F liquid
 TESTNAME="Running cargo clippy check electrum-discovery + liquid"
 echo "$TESTNAME"
 cargo clippy $@ -q -F electrum-discovery,liquid
+
+TESTNAME="Running cargo test with all features"
+echo "$TESTNAME"
+cargo test $@ -q --package electrs --lib --all-features

src/electrum/discovery.rs

@@ -531,20 +531,26 @@ mod tests {
     const PROTOCOL_VERSION: ProtocolVersion = ProtocolVersion::new(1, 4);
 
     #[test]
+    #[ignore = "This test requires external connection to server that no longer exists"]
     fn test() -> Result<()> {
         stderrlog::new().verbosity(4).init().unwrap();
 
+        #[cfg(feature = "liquid")]
+        let testnet = Network::LiquidTestnet;
+        #[cfg(not(feature = "liquid"))]
+        let testnet = Network::Testnet;
+
         let features = ServerFeatures {
             hosts: serde_json::from_str("{\"test.foobar.example\":{\"tcp_port\":60002}}").unwrap(),
             server_version: VERSION_STRING.clone(),
-            genesis_hash: genesis_hash(Network::Testnet),
+            genesis_hash: genesis_hash(testnet),
             protocol_min: PROTOCOL_VERSION,
             protocol_max: PROTOCOL_VERSION,
             hash_function: "sha256".into(),
             pruning: None,
         };
         let discovery = Arc::new(DiscoveryManager::new(
-            Network::Testnet,
+            testnet,
             features,
             PROTOCOL_VERSION,
             false,

src/new_index/mempool.rs

@@ -422,7 +422,7 @@ impl Mempool {
         for txid in txids {
             let tx = self.txstore.get(&txid).expect("missing tx from txstore");
 
-            let prevouts = match extract_tx_prevouts(tx, &txos, false) {
+            let prevouts = match extract_tx_prevouts(tx, &txos) {
                 Ok(v) => v,
                 Err(e) => {
                     warn!("Skipping tx {txid} missing parent error: {e}");

src/new_index/query.rs

@@ -127,6 +127,7 @@ impl Query {
             .or_else(|| self.mempool().lookup_raw_txn(txid))
     }
 
+    /// Not all OutPoints from mempool transactions are guaranteed to be included in the result
     pub fn lookup_txos(&self, outpoints: &BTreeSet<OutPoint>) -> HashMap<OutPoint, TxOut> {
         // the mempool lookup_txos() internally looks up confirmed txos as well
         self.mempool().lookup_txos(outpoints)

src/rest.rs

@@ -155,9 +155,9 @@ impl TransactionValue {
         blockid: Option<BlockId>,
         txos: &HashMap<OutPoint, TxOut>,
         config: &Config,
-    ) -> Self {
-        let prevouts =
-            extract_tx_prevouts(&tx, txos, true).expect("Cannot Err when allow_missing is true");
+    ) -> Result<Self, errors::Error> {
+        let prevouts = extract_tx_prevouts(&tx, txos)?;
+
         let vins: Vec<TxInValue> = tx
             .input
             .iter()
@@ -174,9 +174,9 @@ impl TransactionValue {
 
         let fee = get_tx_fee(&tx, &prevouts, config.network_type);
 
-        TransactionValue {
+        #[allow(clippy::unnecessary_cast)]
+        Ok(TransactionValue {
             txid: tx.txid(),
-            #[allow(clippy::unnecessary_cast)]
             version: tx.version as u32,
             locktime: tx.lock_time,
             vin: vins,
@@ -185,7 +185,7 @@ impl TransactionValue {
             weight: tx.weight() as u32,
             fee,
             status: Some(TransactionStatus::from(blockid)),
-        }
+        })
     }
 }
 
@@ -519,6 +519,9 @@ fn ttl_by_depth(height: Option<usize>, query: &Query) -> u32 {
     })
 }
 
+/// Prepare transactions to be serialized in a JSON response
+///
+/// Any transactions with missing prevouts will be filtered out of the response, rather than returned with incorrect data.
 fn prepare_txs(
     txs: Vec<(Transaction, Option<BlockId>)>,
     query: &Query,
@@ -537,7 +540,7 @@ fn prepare_txs(
     let prevouts = query.lookup_txos(&outpoints);
 
     txs.into_iter()
-        .map(|(tx, blockid)| TransactionValue::new(tx, blockid, &prevouts, config))
+        .filter_map(|(tx, blockid)| TransactionValue::new(tx, blockid, &prevouts, config).ok())
         .collect()
 }
 
@@ -1003,7 +1006,15 @@ fn handle_request(
 
             let mut tx = prepare_txs(vec![(tx, blockid)], query, config);
 
-            json_response(tx.remove(0), ttl)
+            if tx.is_empty() {
+                http_message(
+                    StatusCode::INTERNAL_SERVER_ERROR,
+                    "Transaction missing prevouts",
+                    0,
+                )
+            } else {
+                json_response(tx.remove(0), ttl)
+            }
         }
         (&Method::GET, Some(&"tx"), Some(hash), Some(out_type @ &"hex"), None, None)
         | (&Method::GET, Some(&"tx"), Some(hash), Some(out_type @ &"raw"), None, None) => {

src/util/transaction.rs

@@ -85,25 +85,21 @@ pub fn is_spendable(txout: &TxOut) -> bool {
 pub fn extract_tx_prevouts<'a>(
     tx: &Transaction,
     txos: &'a HashMap<OutPoint, TxOut>,
-    allow_missing: bool,
 ) -> Result<HashMap<u32, &'a TxOut>, errors::Error> {
     tx.input
         .iter()
         .enumerate()
         .filter(|(_, txi)| has_prevout(txi))
-        .filter_map(|(index, txi)| {
-            Some(Ok((
+        .map(|(index, txi)| {
+            Ok((
                 index as u32,
-                match (allow_missing, txos.get(&txi.previous_output)) {
-                    (_, Some(txo)) => txo,
-                    (true, None) => return None,
-                    (false, None) => {
-                        return Some(Err(
-                            format!("missing outpoint {:?}", txi.previous_output).into()
-                        ));
+                match txos.get(&txi.previous_output) {
+                    Some(txo) => txo,
+                    None => {
+                        return Err(format!("missing outpoint {:?}", txi.previous_output).into());
                     }
                 },
-            )))
+            ))
         })
         .collect()
 }