Duplicate input detection

[R27-pixel]

Description and Notes

This PR adds duplicate input detection inside check_transaction_context_free(), closing a missing consensus-level validation gap.

Floresta had no context-free check for duplicate inputs. A transaction spending the same OutPoint twice could pass consensus checks and proceed further into verification. Mempool tests were also constructing duplicate inputs inside a single transaction, which masked the missing consensus validation.

Bitcoin Core performs this check in CheckTransaction() (see CVE-2018-17144) because missing it can cause crashes or inflation bugs depending on UTXO handling.

In Floresta, get_utxo() uses .get() on the UTXO map, so the same UTXO value could be counted twice toward in_value, creating a potential inflation vector.

Changes

• Added DuplicateInput to BlockValidationErrors with a corresponding Display implementation
• Added a HashSet-based duplicate prevout check in check_transaction_context_free()
• Placed the check after null prevout validation and before output value checks, matching Bitcoin Core’s ordering
• Updated mempool tests to use real cross-transaction conflicts instead of invalid single-transaction duplicate inputs, and changed the expected error from DuplicatedInputs to ConflictingTransaction
• Added handling for DuplicateInput in floresta-wire block validation flow
• Replaced witness-size TODO with an explicit comment clarifying that witness size is intentionally excluded from context-free checks, matching Bitcoin Core’s CheckTransaction() behavior

How to verify the changes you have done?

cargo fmt
cargo test --all
cargo test test_duplicate_inputs_rejected
cargo test test_gbt_with_conflict

[Davidson-Souza]

verify_transaction already checks for this, because when we validate an input we remove the UTXO from it. If the inputs are duplicated, the second time will fail on a missing prevout.

[R27-pixel]

verify_transaction already checks for this, because when we validate an input we remove the UTXO from it. If the inputs are duplicated, the second time will fail on a missing prevout.

Thanks ft pointing this, i checked the exact code(please correct me if im wrong)

The .remove() happens inside verify_input_script(), which is behind # [cfg(feature="bitcoinkernel")]

fn verify_input_scripts( 
      transaction: &Transaction,
      utxos: &mut HashMap<OutPoint, UtxoData>, 
      flags: c_uint, ) -> Result<(), BlockchainError> {
                            
       let spent_output = utxos                 
             .remove(&input.previous_output)
        }

so the protection exists only when the bitcoinkernel is enabled.
in nrml verify_transaction() , inputs are validate through:
let utxo = Self::get_utxo(input, utxos, txid)?;
and the get_utxo fn uses: match utxos.get(&input.previous_output)
which only reads it.
means if the same outpoint apprs two times in a single tx, both will succed and the same utxo value will be counted two times towars in_value, creating dup-input issue bitcoin core fixed in CheckTransaction() (CVE-2018-17144)

THis pr detects duplicate input, so the tx is rejected unconditionaly accros all build confg, and also matching bitcoin cores's behavior for duplicate input handling.

[Davidson-Souza]

verify_transaction already checks for this, because when we validate an input we remove the UTXO from it. If the inputs are duplicated, the second time will fail on a missing prevout.

Thanks ft pointing this, i checked the exact code(please correct me if im wrong)

The .remove() happens inside verify_input_script(), which is behind # [cfg(feature="bitcoinkernel")]

fn verify_input_scripts( 
      transaction: &Transaction,
      utxos: &mut HashMap<OutPoint, UtxoData>, 
      flags: c_uint, ) -> Result<(), BlockchainError> {
                            
       let spent_output = utxos                 
             .remove(&input.previous_output)
        }

so the protection exists only when the bitcoinkernel is enabled. in nrml verify_transaction() , inputs are validate through: let utxo = Self::get_utxo(input, utxos, txid)?; and the get_utxo fn uses: match utxos.get(&input.previous_output) which only reads it. means if the same outpoint apprs two times in a single tx, both will succed and the same utxo value will be counted two times towars in_value, creating dup-input issue bitcoin core fixed in CheckTransaction() (CVE-2018-17144)

THis pr detects duplicate input, so the tx is rejected unconditionaly accros all build confg, and also matching bitcoin cores's behavior for duplicate input handling.

Hmm, now I think you are onto something! If libitcoinkernel isn't enabled, we might not check for duplicated inputs. I think a good way to fix this would be to remove the utxos inside verify_transactions, since we wouldn't need to create another hash set just for this.

[R27-pixel]

verify_transaction already checks for this, because when we validate an input we remove the UTXO from it. If the inputs are duplicated, the second time will fail on a missing prevout.

Thanks ft pointing this, i checked the exact code(please correct me if im wrong)
The .remove() happens inside verify_input_script(), which is behind # [cfg(feature="bitcoinkernel")]

fn verify_input_scripts( 
      transaction: &Transaction,
      utxos: &mut HashMap<OutPoint, UtxoData>, 
      flags: c_uint, ) -> Result<(), BlockchainError> {
                            
       let spent_output = utxos                 
             .remove(&input.previous_output)
        }

so the protection exists only when the bitcoinkernel is enabled. in nrml verify_transaction() , inputs are validate through: let utxo = Self::get_utxo(input, utxos, txid)?; and the get_utxo fn uses: match utxos.get(&input.previous_output) which only reads it. means if the same outpoint apprs two times in a single tx, both will succed and the same utxo value will be counted two times towars in_value, creating dup-input issue bitcoin core fixed in CheckTransaction() (CVE-2018-17144)
THis pr detects duplicate input, so the tx is rejected unconditionaly accros all build confg, and also matching bitcoin cores's behavior for duplicate input handling.

Hmm, now I think you are onto something! If libitcoinkernel isn't enabled, we might not check for duplicated inputs. I think a good way to fix this would be to remove the utxos inside verify_transactions, since we wouldn't need to create another hash set just for this.

Thats a valid approach, like changing get_utxo/verify_tx to use .remove would naturally catch duplicates but maybe i think the tradeoff is that the current approach will catch it earlier in check_transaction_context_free(), before touching utxo mapping (also it matches the bitcoin core design the checktransaction fn one where duplicte inputs are rejected as a strctual prop, independent of any chain state).
But i agree that the .remove approach is simple and doesnt need extra hashset. also verify_block_transactions() already owns the UTXO map, so mutation there is fine.

happy to update the pr either way, pls let me know which approach is preferable.

[R27-pixel]

Hii @Davidson-Souza Just a gentle ping on this PR whenever you get time to discuss or review. Thanks!!

[Davidson-Souza]

I prefer the latter (not using an extra hash set). Perhaps we could have an issue to discuss this mo in-depth? PRs are usually not the place for deep discussions.

[moisesPompilio]

need rebase

[R27-pixel]

need rebase

rebased.

[moisesPompilio]

I don’t understand the reason for this here.

[R27-pixel]

The used_in_this_tx guard is necessary since check_transaction_context_free() now rejects any transaction containing duplicate inputs.

In the original build_transactions(..., true) code, transactions with duplicate inputs were inadvertently being created because the same outpoint could be reused multiple times inside the inner loop. Once the consensus validation was added, test_gbt_with_conflict started failing with DuplicateInput before the mempool conflict logic even kicked in.

So i added guard which makes sure that each outpoint appears only once within a single transaction.

[moisesPompilio]

In the mempool this does not make sense, because in the mempool there can be RBF.

[R27-pixel]

In the mempool this does not make sense, because in the mempool there can be RBF.

To clarify, used_in_this_tx is in build_transactions(), which is a test helper function that creates fake transactions. This is not related to the mempool logic at all and does not affect RBF.

RBF requires two valid transactions competing against one another in the mempool, and that behavior is untouched by this PR.

This is just a guard that stops build_transactions() from creating an invalid transaction that uses the same input twice in a transaction.

However, I can see where you're coming from and am happy to follow your recommendation on closing this and waiting until the discussion in the issue is over.

[moisesPompilio]

I think it’s better to close this PR because it is not mature enough. There is still an open issue to discuss this problem.

[moisesPompilio]

In the mempool this does not make sense, because in the mempool there can be RBF.