Use REJECT_DUPLICATE for already known and conflicted txn #10503

1. 
   sipa commented at 6:15 pm on June 1, 2017: member

   Per BIP61, reject code 0x12 for transactions corresponds to “input already spent”.

   We currently have two internal codes (REJECT_CONFLICT and REJECT_ALREADY_KNOWN) for validation failures for transactions that conflict with the mempool, and are already known repectively.

   I think that under a reasonable interpretation of BIP61, those failures actually match “input already spent” and should thus trigger a reject code 0x12.

   Implement that.

   Reported by @achow101 that some failures did not cause a reject message.

2. 
   gmaxwell commented at 6:21 pm on June 1, 2017: contributor

   I would rather remove P2P reject codes; they waste bandwidth and AFAIK aren’t actually used for anything except perhaps our tests where they are only moderately helpful due to their narrow bandwidth and instability. (if your change doesn’t make tests fail, it would further show their lack of use).
3. 
   laanwj commented at 6:37 pm on June 1, 2017: member

   Concept ACK @gmaxwell I think it would be preferable if you create an issue for removing reject codes, instead of replying that to every PR that changes something to reject code handling. It’s harder to keep track of it if it’s all over the place - we had this discussion recently: #10135 (comment)
4. 
   jonasschnelli commented at 6:40 pm on June 1, 2017: contributor

   utACK 49be502dcc890f5d00930661e7ec926e71489bdb I guess @achow101 got the rejection code via RPC sendrawtransaction. There it makes sense IMO.
5. jonasschnelli added the label Mempool on Jun 1, 2017
6. 
   gmaxwell commented at 6:41 pm on June 1, 2017: contributor

   @laanwj fair request, but I really had hoped to avoid having to think about if additional disclosure hurts privacy for a feature that I think we should be backing away from. OK.
7. in src/validation.cpp:433 in 49be502dcc outdated

   427@@ -428,7 +428,7 @@ bool AcceptToMemoryPoolWorker(CTxMemPool& pool, CValidationState& state, const C
   428 
   429     // is it already in the memory pool?
   430     if (pool.exists(hash))
   431-        return state.Invalid(false, REJECT_ALREADY_KNOWN, "txn-already-in-mempool");
   432+        return state.Invalid(false, REJECT_DUPLICATE, "txn-already-in-mempool");
   433 
   

   ---

   ---

   

   gmaxwell commented at 6:45 pm on June 1, 2017:

   I would have added braces on this and the other lines that are being changed to update the style.

   ---

   

   sipa commented at 9:59 pm on June 1, 2017:

   Fixed.
8. sipa force-pushed on Jun 1, 2017
9. 
   gmaxwell commented at 7:15 am on June 2, 2017: contributor

   Needs rebase.
10. Use REJECT_DUPLICATE for already known and conflicted txn d9bec888f1
11. 
    sipa commented at 7:19 am on June 2, 2017: member

    Rebased.
12. sipa force-pushed on Jun 2, 2017
13. 
    TheBlueMatt commented at 0:48 am on June 5, 2017: member

    I believe there are a few clients out there that do actually use this crap, at least I recall breadwallet did at some point to identify issues with txn they were sending to peers. Probably should go through a more formal announce/BIP process for these kinds of changes, sadly :(.
14. 
    achow101 commented at 6:47 pm on June 5, 2017: member

    One thing that this does fix is the fact that a reject message wasn’t being sent if a transaction conflicted with another one that was already in the mempool. A reject message should be sent for such a scenario to let the sending peer know that there is a double spend attempt going on. I noticed this while doing some debugging with Armory.
15. 
    laanwj commented at 8:59 am on June 6, 2017: member

    Probably should go through a more formal announce/BIP process for these kinds of changes, sadly :(.

    This change, as it is, just adds visible reject messages for cases where the node is silent now. As it is in the spirit of BIP61 I don’t think this requires a new BIP. It would be nice to mention in the release notes, though.

16. 
    sipa commented at 5:23 pm on June 6, 2017: member

    @TheBlueMatt The current implementation violates BIP61, in my interpretation.
17. 
    achow101 commented at 11:05 pm on June 6, 2017: member

    utACK d9bec888f1c82f1f58cc821cac81da9c571b5fa5
18. dcousens approved
19. 
    dcousens commented at 0:58 am on June 7, 2017: contributor

    utAck
20. 
    TheBlueMatt commented at 5:17 pm on June 7, 2017: member

    @laanwj Ah, missed that, I withdraw my objection.

    utACK d9bec888f1c82f1f58cc821cac81da9c571b5fa5

21. sipa merged this on Jun 21, 2017
22. sipa closed this on Jun 21, 2017

    ---

23. sipa referenced this in commit efbcf2b1d5 on Jun 21, 2017
24. sipa deleted the branch on Jun 23, 2017
25. PastaPastaPasta referenced this in commit eaf2d35d7f on Jul 5, 2019
26. PastaPastaPasta referenced this in commit 6ce5b32901 on Jul 5, 2019
27. PastaPastaPasta referenced this in commit 01b0e95dd7 on Jul 6, 2019
28. PastaPastaPasta referenced this in commit 5c11821755 on Jul 8, 2019
29. PastaPastaPasta referenced this in commit cf57d70815 on Jul 9, 2019
30. PastaPastaPasta referenced this in commit 9d336854f7 on Jul 9, 2019
31. barrystyle referenced this in commit a97fc1855b on Jan 22, 2020
32. DrahtBot locked this on Sep 8, 2021

Contributors
sipa gmaxwell laanwj jonasschnelli TheBlueMatt achow101 dcousens

Labels
Mempool