tx_valid: re-order inputs to how they are encoded #9100

pull dcousens wants to merge 1 commits into bitcoin:master from dcousens:testorder changing 1 files +9 −9
  1. dcousens commented at 12:59 AM on November 8, 2016: contributor

    Just something I noted when using these externally. The new ordering matches how they are encoded in the serialised transaction.

    If there was a reason for these to be ordered this way, then I've probably misinterpreted the test data.

  2. tx_valid: re-order inputs to how they are encoded 5262a151e1
  3. fanquake added the label Tests on Nov 8, 2016
  4. laanwj commented at 9:13 PM on November 9, 2016: member

    If there was a reason for these to be ordered this way, then I've probably misinterpreted the test data.

    That depends: is the re-ordering part of the test or not?

  5. dcousens commented at 1:35 AM on November 10, 2016: contributor

    That depends: is the re-ordering part of the test or not?

    The tests don't fail with this new ordering, and the resultant transaction is encoded the same. There does not appear to be any hints in the associated comments that the ordering was important either.

  6. dcousens commented at 1:38 AM on November 10, 2016: contributor

    if (mapprevOutValues.count(tx.vin[i].prevout)) {

    In the tests, they are just using a map-lookup to see if the (transaction decoded) inputs were in the provided list. Ultimately it is the encoded transaction that was dictating the order (as can be seen in the "fixed" data).

  7. MarcoFalke added the label Refactoring on Nov 11, 2016
  8. MarcoFalke commented at 11:23 AM on November 11, 2016: member

    No strong opinion about changing the order, but I think it makes sense to be consistent, if there was no particular reason for the current order.

  9. MarcoFalke commented at 11:23 AM on November 11, 2016: member

    @NicolasDorier Was there a particular reason you chose that order?

  10. NicolasDorier commented at 10:15 PM on November 11, 2016: contributor

    None, no strong opinion either. I generated the tests from some C# code.

  11. dcousens commented at 2:21 AM on November 12, 2016: contributor

    The motivation for this change was that it was @un-intuitive at first why my tests were failing in an external open-source library that was testing against these inputs 1:1 (that is, I had an underlying assumption of the inputs being in-order).

    Putting the test fixtures in order also reduces the potential complexity of a test by allowing the inputs to be matched with a simple for loop rather than doing a double-iteration or map-lookup.

  12. dcousens commented at 9:22 AM on November 25, 2016: contributor

    Anything further needed here?

  13. MarcoFalke merged this on Nov 25, 2016
  14. MarcoFalke closed this on Nov 25, 2016

  15. MarcoFalke referenced this in commit 97ec6e5c90 on Nov 25, 2016
  16. dcousens deleted the branch on Nov 25, 2016
  17. codablock referenced this in commit 8f0118a8a5 on Jan 16, 2018
  18. codablock referenced this in commit b5477088ad on Jan 16, 2018
  19. codablock referenced this in commit 2c8fe46868 on Jan 17, 2018
  20. andvgal referenced this in commit 36cd8ee5eb on Jan 6, 2019
  21. CryptoCentric referenced this in commit 5946bc01de on Feb 25, 2019
  22. DrahtBot locked this on Sep 8, 2021

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-22 06:15 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me