P2P: Mempool tracks locally submitted transactions to improve wallet privacy #18038

• added logic to save unbroadcast set to disk
• added thread safety annotation & missing locks from lots of call sites 😛

thank you both for this feedback 🙏🏽 would love further review on the added logic

from #18038 (comment)

RE: wtxid - there is some additional code needed to support unbroadcast set & wtxid relay. I’ll work on the patch & depending on timing around which PR is ready for merge first, include the patch here or offer it on the other PR. Thanks again for pointing that out :)

I don’t fully understand what you mean here. Are you pointing out that there are other ways the transaction gets removed from the set?

I updated to: “We intepret processing a GETDATA for a transaction as a successful initial broadcast and remove it from our unbroadcast set.”

Does that seem reasonable? Or am I still missing the point?

Why is the tx not removed when it was initially sent out after 15 minutes, e.g. when it was removed from mapRelay and then relayed from the mempool.

I think this code should go down into the if(push) branch

I updated it to “from m_unbroadcast_txids before confirmation that txn was sent out”, but I feel uncertain… saying “before GETDATA was received” seems more clear & specific than “confirmation that txn was sent out”. Could you explain further why you didn’t like it?

is this related to this?

updated.

funny to update the code to match the comment language rather than vice versa 😛 but I don’t have a strong preference around 12-36hrs vs 0-24hrs, both have small tradeoffs.

It might. Which upholds the same guarantees as the previous logic it’s replacing.

On current master, wallet transactions get rebroadcast through a scheduler flow that makes its way to ResendWalletTransactions, which selects transactions to rebroadcast then invokes SubmitMemoryPoolAndRelay. This goes to BroadcastTransaction, which checks ATMP then relays via RelayTransaction, which uses the CNode#PushInventory function that only adds the hash if its not contained in filterInventoryKnown.

This PR changes the initial parts of the flow… so BroadcastTransaction adds the transaction to mempool.m_unbroadcast_txids, then there is a scheduled job to periodically AttemptInitialBroadcast, but the behavior is matched where the locally-submitted transactions would be reattempted to send to the network, filtered for the connections that were already tried, and sent out to new connections.

Changing that behavior could be desirable for robustness but of course reduces privacy. So out of scope for this PR. If you think it may be desirable, I’d be happy to think it through / talk about it more, we can open a new issue. (I’m working on the assumption that current delivery guarantees are sufficient).

Maybe it’s the naming that confuses me. It’s called “rebroadcast”, not “consider to rebroadcast”.

even if you did, those peers would probably still not send a GETDATA anyway

Not sure about that. If transaction expires from a mempool, and there’s not much activity on-chain, filter would still prevent us from re-announcing it. The correspondence between the two is questionable.

But yeah, if that’s only my own confusion, I don’t care about just keeping it this way without any comments.

Maybe it’s the naming that confuses me. It’s called “rebroadcast”, not “consider to rebroadcast”.

the language I’m introducing is “unbroadcast”, which means consider it as initial broadcast. but there are these interactions with filterInventoryKnown to consider in different cases.

I added some comments here in an attempt to make this more apparent.

going to resolve this conversation, but @naumenkogs let me know if you think this is something we need to more carefully consider in regards to this PR.

ah, I thought through the possible privacy leaks but didn’t think about weird circumstances that could lead to synchronized network-wide events.

updated reattempts to be between 10-15 minutes, chosen every time a node starts up.

in commit 77748da2269bd7f2304ebc3b8616d3a584e7c65f:

Can be written shorter as

0                if (WITH_LOCK(mempool.cs, return mempool.m_unbroadcast_txids.erase(inv.hash);)) {

in commit 77748da:

Please use node.mempool and not the global

in commit 2f3f449dc9c89420a444cc8263c5a5fe537a475b:

I still haven’t figured out a way to exploit this for privacy leaks, but making each node pick a unique fingerprint (heartbeat between 10-15 minutes) doesn’t seem too great. Sometimes nodes offer a tor p2p interface and a clearnet p2p interface and ideally those should be uncorrelated. Obviously horribly broken in the current p2p implementation, but we shouldn’t make it worse, I think.

So what do you think about just scheduling this once here and then reschedule this at the end of the AttemptInitialBroadcast function?

Something like

0void CConnman::ScheduleNextInitialBroadcast(CScheduler& s) const
1{
2 const auto delta = std::chrono::minutes{10} + GetRandMicros(std::chrono::minutes{5});
3 s.schedule([&]{this->AttemptInitialBroadcast(s);}, delta);
4}

yeah great point. p2p fingerprints are never good. thanks for pointing out.

having AttemptInitialBroadcast call ScheduleNextInitialBroadcast to schedule the next AttemptInitialBroadcast seems a bit much to me (and I’d have to pass the scheduler through anyways), so I’ll just schedule the next invocation at the end of the function itself.

in commit 2f3f449dc9c89420a444cc8263c5a5fe537a475b:

style nit. Can be a reference because it is never null

0void PeerLogicValidation::QueueUnbroadcastTxs(CNode& pnode)

in commit 2f3f449dc9c89420a444cc8263c5a5fe537a475b:

Please use m_mempool and not the global.

in commit 2f3f449:

I don’t understand this comment, why would it have weaker delivery guarantees? Is it the bloom filter false positive rate?

no, simpler.. just mean some transactions will get filtered & not relayed because of filterInventoryKnown. this was a result of the confusion & conversation in #18038 (review).

but maybe without that context this comment is misleading? appreciate any feedback on how to clarify. right now I’m thinking of just deleting the second paragraph.

in commit 2f3f449:

Can be const?

0    void QueueUnbroadcastTxs(CNode& node) const override;

in commit f40f6875f5570cb25882d8a04a6cbeb61ee7366d:

Style nit. Could add trailing comma for symmetry, git blame, and code formatters?

0        hex_str_to_bytes,

In commit f40f6875f5570cb25882d8a04a6cbeb61ee7366d:

I think assert is a statement, not a function, so should be without ()

0        assert wallet_tx_hsh not in mempool

in commit f40f6875f5570cb25882d8a04a6cbeb61ee7366d:

Could do the same test again with a self.nodes[0].restart_node() added here?

This way it will test to persist txs on disk.

in commit f40f6875f5570cb25882d8a04a6cbeb61ee7366d:

Can be shorter, because self.sync_mempools() does the same, I think

0        self.sync_mempools()

in commit f40f687:

Does the block need to be created manually? What about a simple two-liner?

0self.nodes[0].sendtoaddress()
1self.nodes[0].generate(1)

Alternatively, if you want to test both wallet txs and rpc txs, you could factor out the tx-creation code from the previous test case into a new helper function and call it here.

ahahhahaha omg I’m glad its been a while since I wrote this test & I don’t remember how long I spent figuring out how to manually make the txn and block 😝

two-liner is much cleaner. I feel comfortable with the coverage of wallet & rpc from previous test.

thanks!

in commit b0f6a238335d96c9f5f7c2b83aabd56ab4f47a71:

Why is this needed?

I think you can just go ahead and assume that the set was serialized. If it wasn’t it will result in a one-time LogPrintf that the mempool data couldn’t be deserialized. I think this is fine if it is mentioned in the release notes or as a code comment or not at all.

so, are you proposing

• remove writing the boolean has_unbroadcast_txns to the file here
• remove try block above, just leave the file>>pool.m_unbroadcast_txids; code below
• if there are no m_unbroadcast_txids, there will be a LogPrintf that “Failed to deserialize mempool data on disk. Continuing anyway”, and code moves on?

if I am interpreting correctly…. I find that to be very misleading user output. And I’d expect it to happen frequently since the unbroadcast set is more of a safety mechanism and hopefully nodes aren’t often shutting down without their local transactions being broadcast to the network. So I’m not really seeing the reasoning / benefits. But maybe I’m misunderstanding your suggestion.

See here:

https://github.com/bitcoin/bitcoin/blob/5236b2e267a58870239673c7ec85e5df0cb8fc8e/src/serialize.h#L972

Why does it have to drop all transactions?

When bumping the version here and assuming the code is merged into 0.21.0, then reading a mempool.dat created by Bitcoin Core 0.21.0 will fail completely on a Bitcoin Core 0.20.0, because the code is already released and the logic of that code is to drop all transactions when the version is not equal to 1.

but the version shouldn’t be bumped because that will silently drop all transactions of the mempool @MarcoFalke - this is interesting. It means if we ever have to bump the version number we’d have to do something like:

• Bitcoin Core version X only understands mempool.dat v1
• add mempool.dat v2 parsing logic in Bitcoin Core version X+1
• add mempoo.dat v2 writing logic in Bitcoin Core version X+2

I think to avoid the debug LogPrint we can wrap the m_unbroadcast_txids parsing in its own try-except. Something like:

0        try {
1            LOCK(pool.cs);
2            file >> pool.m_unbroadcast_txids;
3            unbroadcast = pool.m_unbroadcast_txids.size();
4        } catch (const std::exception& e) {
5            // mempool.dat files created prior to v0.21 will not have an
6            // unbroadcast set. No need to log a failure if parsing fails
7            // here.
8        }

I think to avoid the debug LogPrint we can wrap the m_unbroadcast_txids parsing in its own try-except

I considered doing this but decided not to because a one-time error message (with functionality working fine) seems more desirable than a nested try / catch statement that would probably live in the code for a very long time.

e82fdd329c812623c6e8a9022322fe43e1000c5b

0                    LogPrint(BCLog::NET, "Removed %i from the set of unbroadcast txs\n", inv.hash.GetHex());

Any reason for the trailing whitespace?

Also, leaking source code names into the debug log might cause parse issues when the symbol is renamed

0        LogPrint(BCLog::NET, "Removed %i from set of unbroadcast txs before confirmation that txn was sent out\n", hash.GetHex());

Same

This should probably be a doxygen comment. See the developer notes on how to get one.

0    /** track locally submitted transactions & periodically retry initial broadcast */

a11968c8780424ddb0fd01341101bb28ff6953a8

Sorry, I still don’t understand why this comment is relevant to rebroadcasting. I remember in an earlier discussion you mentioned that a user might disconnect their wifi. A tx might get added to filterInventoryKnown, but then never sent out.

I don’t think this is true. Either, we reconnect the wifi and be still connected to the peer, in which case we can push the inv out and wait for them to request the tx. Or, we drop the peer (either because the wifi isn’t turned on again, or it took us too long to do so), in which case a new peer will pop up and they will receive the inv (after 10-15 minutes).

we reconnect the wifi and be still connected to the peer, in which case we can push the inv out and wait for them to request the tx.

ah, I didn’t realize. thanks!

removed the comment.

0    const std::chrono::milliseconds delta = std::chrono::minutes{10} + GetRandMillis(std::chrono::minutes{5});

You might have to add GetRandMillis by copying from GetRandMicros.

6307582b4d30167a931678d12e99a00a0e75e81a

This is not safe. You are erasing the existing content of the m_unbroadcast_txids

😢

unfortunately I can’t disagree that its silly to touch a file just to remove whitespace.

empty lines in net.cpp… I will be back for you!

I prefer all classes in the framework to have a docstring, even if it’s very simple. This one would be something like:

“A P2PInterface which stores a count of how many times each txid has been announced”

good point! looks like since P2PTxInvStore overwrites the on_inv function, it doesn’t send back a GETDATA, so the relevant piece of functionality from a connection isn’t triggered.

for this PR, I added a line to disconnect_p2ps so the expectations make more sense (making the comment true).

but I do think its confusing that P2PTxInvStore inherits from P2PInterface but abandons this one piece of functionality. I’m thinking as a small followup I can have P2PTxInvStore call through to the parent on_inv after it stores the txid to behave more as expected. what do you think?

You are missing the parenthesis behind the function call: node.disconnect_p2ps() so this line is not doing the disconnecting at the moment. So the comment is indeed wrong and it just “accidentally” works because P2PTxInvStore doesn’t send a GETDATA. I think I would expect it to send a GETDATA because it is part of the framework now, but in general, I think the P2PInterface is there so you can override the behavior to be what you need so I don’t have a strong feeling about it. If it was part of the test I would say it doesn’t matter but because it moves to the framework, slight preference to call the super behavior.

EDIT: For reference, I mean this line: https://github.com/bitcoin/bitcoin/pull/18038/files#diff-5e6f5e6aab04f82ec4b58148c4bdec72R85 the linking isn’t working properly because I commented on an outdated conversation.

🤦‍♀️ sigh. thanks for catching!

ya I agree with the reasoning of in test vs framework. I’ll do a small followup to add parens & have P2PTxInvStore call through super.

This doesn’t work. You can try by modifying the string above

The type needs to be an array

Any reason why these are inline? I think we don’t that elsewhere.

Also, it might be good to move the implementation of this one to the cpp file, so that logging doesn’t have to be included in the header and compiled on every include.

thought it was more efficient since these functions are so simple.

we don’t have inline functions in txmempool, but there are plenty scattered around the codebase.

👍 RE logging. I can def move this function to the cpp. Do you prefer I move them all?

@amitiuttarwar inline has two independent effects:

• A hint to the compiler that it should try inlining the function (both in C and C++). Modern compilers will do this automatically in many cases, with or without the keyword.
• Indicating that this symbol may appear in multiple modules (but they will all be identical). This is only in C++, and makes it possible to have code in header files (as it will end up in every module that includes it). Member functions defined in classes always have this implicitly, no need to add the “inline” keyword for it (unless you also want to hinting effect above).

So the only time you have to use inline is for functions in headers files that are not member functions.

but, since I’m pulling out the definition of RemoveUnbroadcastTx to txmempool.cpp, it will no longer be automatically inlined. So, adding inline there would be a hint to the compiler to inline the function at the call sites, but the compiler might do it anyway / ignore.

is this sounding correct?

that the symbol defined in more than one translation unit are the same

Not just that they’re the same; that they may occur in multiple translation units in the first place. Without inline (or a member being defined in a class body), you will get a linker error if you define the same symbol in multiple translation units.

Unless (there’s always an unless…), it’s in an anonymous namespace as well (or static for globals). In that case, every translation unit will get its own individual copy, which won’t be seen as conflicting by the linker at all - but they also won’t get deduplicated.

ok ya, got the part where inline indicates permission for same symbol in multiple translation units. the part about inline in .cpp being limited to the file unless the optimization is enabled makes sense. and the special case for anonymous namespace is 🤯 and I’m going to go have to learn / tinker more 😛

but first I’m going to push fixups. thanks so much for these explanations! I’m learning a lot :)

I’m not fully following either, but a couple of points to highlight:

• lets distinguish unbroadcast from rebroadcast. unbroadcast is extra attempts at successful initial broadcast. rebroadcast is repeated broadcasts (from the POV of the node, which cannot know full picture).
• in this implementation, if victim sends INV to only one peer, peer is adversarial, replies back with GETDATA but doesn’t propagate it out to the network -> victim removes from unbroadcast set & doesn’t retry until ~1 day later when ResendWalletTransactions is triggered. so, prevents txn from making it out to mempool for a day.
• @naumenkogs do you think unsolicited GETDATA messages would change this?

Imagine a lightning node submits a tx, but it is offline, and will be online in 30 minutes to relay it from Unbroadcast. Now an attacker connect to the victim once they got online, and send GETDATA for that transaction (before a victim broadcasts from unbroadcast).

Right now, it will be ignored, because it’s unsolicited (for privacy reasons!). If we discard those privacy reasons, and now forget about this inherent unbroadcast behaviour, and allow unsolicited GETDATA, an attacker will be able to prevent us from broadcasting a tx, by removing a tx from unbroadcast.

And yeah, one day for lightning is pretty bad, you will simply loose money in most of the implementations today.

Yes that’s a good point, in LN there is class of transaction where txid is known from remote party due to the fact they counter-sign them (HTLC-timeout) but honest party have to broadcast with time-constrain to avoid any risk of money-loss.

Right now most of implementations are pretty aggressive at triggering rebroadcast at every block, i.e calling again sendrawtransaction which isn’t great for privacy and this may change in the future. That said, often offline nodes, like mobile, won’t use core as a backend and broadcast directly their transaction on the p2p layer or submit to a third-party server. Routing nodes assumed to run core, should rebroadcast at every block so it would override some attacker messing with their unbroadcast logic.

I think adopting wtxid would solve this issue because counterparty won’t know about witness, at least for LN. But I agree, we should document this to keep track (add a comment commit on top or as a follow-up)

ok, updating to check pfrom is from outbound sounds good to me. increases delivery guarantee for the general case too. I added this to the todo list on follow ups pr #18807#issue-410374742. code is just 1 line change, but I want to make sure to update other relevant comments to clarify its removed via outbound getdata & hopefully add a test case.

I’m going to resolve this comment here. thanks!

on further thought, I don’t think it makes sense to only remove unbroadcast txns on receiving GETDATA from outbound peers.

this relies on the expectation that outbound peers get the txn before inbound peers (otherwise they might relay), which isn’t necessarily true in edge cases. if we were to implement, I’d want to add logic to remove from unbroadcast set if we receive an inv from a peer. However, doing all this seems unnecessary right now, especially considering the greater direction of this project (#16698).

89eeb4a

Should we annotate this new methods EXCLUSIVE_LOCKS_REQUIRED(cs_main, cs) to increase confidence that lock order is preserved ? (like some of them are called from node helpers code and we may refactor lock tacking there at some point)

89eeb4a

Should we annotate this new methods EXCLUSIVE_LOCKS_REQUIRED(cs_main, cs) to increase confidence that lock order is preserved ? (like some of them are called from node helpers code and we may refactor lock tacking there at some point)

Was asking myself this as well.

Maybe the comments were not about lock order, but whether the cs_main lock is needed as per #14963? :thinking:

Haven’t thought about this, but if I had to guess, I’d say no.

e25e42f

You could have comment that rational of recursively calling scheduleFromNow is to get a different delta every time and that way avoid an observable reattempt-period fingerprint.

yes good point. noted here #18807#issue-410374742 and will incorporate into the follow-up. resolving this comment in favor of tracking there.

thanks!

297a178 suggestion

 0+MAX_INITIAL_BROADCAST_DELAY = 15 * 60  # 15 minutes in seconds
 1 
 2 class MempoolUnbroadcastTest(BitcoinTestFramework):
 3     def set_test_params(self):
 4@@ -65,7 +66,7 @@ class MempoolUnbroadcastTest(BitcoinTestFramework):
 5         connect_nodes(node, 1)
 6 
 7         # fast forward into the future & ensure that the second node has the txns
 8-        node.mockscheduler(15 * 60)  # 15 min in seconds
 9+        node.mockscheduler(MAX_INITIAL_BROADCAST_DELAY)  # 15 min in seconds
10         self.sync_mempools(timeout=30)
11         mempool = self.nodes[1].getrawmempool()
12         assert rpc_tx_hsh in mempool
13@@ -74,7 +75,7 @@ class MempoolUnbroadcastTest(BitcoinTestFramework):
14         self.log.info("Add another connection & ensure transactions aren't broadcast again")
15 
16         conn = node.add_p2p_connection(P2PTxInvStore())
17-        node.mockscheduler(15 * 60)
18+        node.mockscheduler(MAX_INITIAL_BROADCAST_DELAY)
19         time.sleep(5)

I think it’s to give the scheduler thread time to wake and call ReattemptInitialBroadcast() again. If you remove the sleep, then the transaction could potentially be broadcast after the assert_equal below and the test wouldn’t test that the tx isn’t being unbroadcast.

I think the sleep could be a bit shorter, and a comment wouldn’t harm, but only if the PR is being retouched.

297a178 this looks like a bug?

0-        node.disconnect_p2ps
1+        node.disconnect_p2ps()

In dc1da48 I think a test is missing here to check that the transaction is not rebroadcast the first 12 hours.

 0        # Check transaction is rebroadcast in 12-36 hour time interval.
 1        time_now = int(time.time())
 2
 3        self.log.info("Bump time 12 hours from now, check transaction is not yet rebroadcast")
 4        rebroadcast_time = time_now  + (12 * 60 * 60)
 5        node.setmocktime(rebroadcast_time)
 6        assert_equal(node.p2ps[1].tx_invs_received[txid], 0)
 7
 8        self.log.info("Bump time 36 hours from now, check transaction is rebroadcast")
 9        rebroadcast_time = time_now + (36 * 60 * 60)
10        node.setmocktime(rebroadcast_time)
11        wait_until(lambda: node.p2ps[1].tx_invs_received[txid] >= 1, lock=mininode_lock)