This looks like the right solution but Is it possible to create a test that fails before the change and passes after? I'd like to make sure an issue like this doesn't sneak in again.

Would it also make sense to fix TxGraph::insert_txout() to not insert duplicates? Since it's a public function I'm worried end users could make the same mistake and insert duplicate txouts and get the same miscalculation. The real problem may be deeper down in the TxGraph::apply_update() call which I believe should prevent creating duplicates in the TxGraph.txs map.

As I understand the behavior of TxGraph, only the first txout inserted will be kept and attempting to insert another one at the same outpoint just returns an empty changeset. In the integration test for electrum the outpoint we're looking for will always have a vout of 0 (they are coinbase txs), so calculating the fee of the received tx happens to work by sheer luck. A more robust test would involve controlling the utxo spent by Core and in which order it appears in the tx which is posing more of a challenge.

Damn this is a huge oversight by @LagginTimes and myself. Thanks for taking this on.

ACK af15ebb

We definitely need more tests for this. We can add these tests in subsequent PRs.

I propose a separate test specific to testing fee calculation that takes into account what @ValuedMammal mentioned in #1443 (comment):

In the integration test for electrum the outpoint we're looking for will always have a vout of 0 (they are coinbase txs), so calculating the fee of the received tx happens to work by sheer luck. A more robust test would involve controlling the utxo spent by Core and in which order it appears in the tx which is posing more of a challenge.