[WIP] new rebroadcast logic

[amitiuttarwar]

implemented in PR

new node-level rebroadcast functionality

• trigger rebroadcasts from the send messages thread, with a poisson delay
• add CTxMemPool::GetRebroadcastTransactions to identify a set of txns the node believes should have been included in a block using BlockAssembler::CreateNewBlock
• add functional tests to ensure txns get rebroadcast between two nodes, correct invs, and that the top txns are correctly chosen and rebroadcast

---

recency filter in block creation

• add idea of BlockAssembler ignoring txns newer than certain age
• enforce minimum to exclude recent txns when selecting rebroadcast candidates
• add functional test

---

replace wallet rebroadcast with submit txns to node

• update wallet ResendWalletTransactions() method to resubmit txns to the mempool rather than rebroadcast to peers
• update associated functional test

---

mempool tracks wallet txns to ensure successful broadcast

• introduce setUnbroadcastTxIDs to CTxMemPool to track wallet txns
• insert txns into the set when submitting to mempool in BroadcastTransaction
• deem a txn as "broadcast" & remove from set when peer sends GETDATA
• on rebroadcast timer, add any unbroadcast txns to setInventoryTxToSend even if they are not at the top of the mempool
• functional test to confirm this behavior

next steps

• run periodic job to cache min fee rate for txn to be included into a block, use that filter to reduce set of txns rebroadcast & add functional test with an emptying out mempool.
• benchmark rebroadcast performance (if deemed important)
• select actual params - and calculate bandwidth expectations / worst case.

[maflcko]

Concept ACK ;)

[jnewbery]

strong concept ACK!

[amitiuttarwar]

this PR is ready for review! some of my thoughts-

1. overall, I don't have a great gauge of how much logging (both in code & tests) is desirable, so I am happy to add more if it makes sense.
2. I wrote a patch to look at "max time" instead of "min age" for the recency filter so I don't call GetTime() for every tx in the mempool. Currently here: amitiuttarwar@0e2ce69. Would appreciate a second pair of eyes to confirm this wouldn't change default behavior.
3. In regards to commit 707a4f5, something I am still thinking about- how to handle the situation where a user submits their txn via the p2p network to their local node & still want the same force-rebroadcast-for-wallet-txns functionality. Possibly a way to exclude peers from being considered "broadcast" peers?
4. Please quadruple check my units of time.

[elichai]

Looks Great :)

[jnewbery]

This is looking really good and I think it's definitely ready to open as a PR in bitcoin/bitcoin.

I've left a lot of comments. They're mostly just style nits, so they shouldn't block you from opening the PR.

Great job! This is really impressive work.

[jnewbery]

One more general comment: you can expand your commit logs a bit. They should be a brief summary of why you're making the change.

[maflcko]

This is looking really good and I think it's definitely ready to open as a PR in bitcoin/bitcoin.

👀 🚀

[maflcko]

I wrote a patch to look at "max time" instead of "min age" for the recency filter so I don't call GetTime() for every tx in the mempool. Currently here: amitiuttarwar/bitcoin@0e2ce69. Would appreciate a second pair of eyes to confirm this wouldn't change default behavior.

I do like that commit

[jnewbery]

I wrote a patch to look at "max time" instead of "min age" for the recency filter

I do like that commit

Looks good to me too

[maflcko]

If you are done with the feedback here, I think this should be closed and all further feedback should go into the pull request in bitcoin/bitcoin. I'd rather not develop new p2p features for Bitcoin Core entirely behind closed doors in a private repo.

[elichai]

FYI, this is not a private repo :)

(your point still stands, just to make sure we all know that this is still all public heh)

[amitiuttarwar]

opened PR on bitcoin/bitcoin & addressing feedback there. closing this PR.