[p2p] miscellaneous wtxid followups #19879

1. 
   amitiuttarwar commented at 10:25 pm on September 4, 2020: contributor

   Addresses some outstanding review comments from #18044

   • reverts unbroadcast txids to a set instead of a map (simpler, communicates intent better, takes less space, no efficiency advantages of map)
   • adds safety around two touchpoints (check for nullptr before dereferencing pointer, remove an inaccurate std::move operator)
   • removes some dead code

   Links to comments on wtxid PR: 1 2 3

   thanks to jnewbery & adamjonas for flagging these ! !

2. [mempool] Revert unbroadcast set to tracking just txid

   When I originally implemented the unbroadcast set in 18038, it just tracked
   txids. After 18038 was merged, I offered a patch to 18044 to make the
   unbroadcast changes compatible with wtxid relay. In this patch, I updated
   `unbroadcast_txids` to a map of txid -> wtxid. Post merge review comments shed
   light on the fact that this update was unnecessary, and distracting. So, this
   commit updates the unbroadcast ids back to a set.

   cb79b9dbf4
3. [p2p] Check for nullptr before dereferencing pointer fc66d0a65c
4. [p2p] Remove dead code

   The else clause is dead code because the only way to not enter the if branch is
   if TX_WITNESS_STRIPPED is true. In that case, it would not have a witness to
   match the `tx.HasWitness()` else condition.
   
   Co-authored-by: Adam Jonas <jonas@chaincode.com>
   Co-authored-by: John Newbery <john@johnnewbery.com>

   125c038126
5. [BroadcastTransaction] Remove unsafe move operator

   Previously, `tx` was being read after having `std::move` called on it. The
   std::move operator indicates to the compiler that this object may be "moved
   from", so we shouldn't subsequently read from it. The current code is not
   problematic since tx is passed in as a const ref. But this `std::move` is at
   best misleading & at worst problematic, so remove it.

   a8a64acaf3
6. fanquake added the label P2P on Sep 4, 2020
7. 
   jnewbery commented at 9:58 am on September 5, 2020: member

   utACK a8a64acaf32ac21feeb885671772282b531ef9a2

   Thanks for tidying up these loose ends! If you retouch, I suggest you update the commit log for the [mempool] Revert unbroadcast set to tracking just txid commit to include the hash of the commit that this is reverting (c7eb6b4f1fe5bd76388a023529977674534334a7).

8. 
   naumenkogs commented at 9:18 am on September 7, 2020: member

   utACK a8a64acaf32ac21feeb885671772282b531ef9a2
9. fanquake requested review from sdaftuar on Sep 15, 2020
10. 
    sdaftuar commented at 5:47 pm on September 15, 2020: member

    utACK a8a64acaf32ac21feeb885671772282b531ef9a2
11. sdaftuar approved
12. fanquake merged this on Sep 15, 2020
13. fanquake closed this on Sep 15, 2020

    ---

14. sidhujag referenced this in commit aae58c49b8 on Sep 16, 2020
15. Fabcien referenced this in commit 1822d571b6 on Nov 16, 2021
16. DrahtBot locked this on Feb 15, 2022

Contributors
amitiuttarwar jnewbery naumenkogs sdaftuar

Labels
P2P