zmq: publish submitted txs via zmq #8427

j-berman · 2022-07-10T03:47:22Z

Problem

When a daemon is in the "fluff" epoch and a user submits a tx to the daemon, the tx doesn't get published via zmq's txpool_add. Reported by @trasherdk; see logs 1, 2, and 3.

Solution

Publish "just broadcasted" txs via zmq.

Of note, the test I added hangs waiting to receive txs that never come over zmq with the code from before this PR.

Deeper context

upon tx submission, handle_incoming_tx is called with relay_method:local with no knowledge of the daemon's fluff/stem epoch status or tx propagation method.
zmq doesn't publish the tx since the tx has not broadcasted yet (and therefore is not visible in the pool).
the daemon then "fluffs" the tx and updates its relay status in the db via on_transactions_relayed making the tx visible to anyone who queries the daemon.
it's possible no other daemons reflect the tx back to the daemon, which means handle_incoming_txs doesn't get called a second time after the initial submission (which is particularly unique to the "fluff" flow).
even if daemons do reflect the tx back to the daemon, since the tx has already updated its relay status to a broadcasted state, it won't publish to zmq since this already_have will be true.

With the above context in mind, I think on_transactions_relayed is the best place to pub txs in this circumstance because:

It's the point we know txs are just set to be broadcasted, and become visible/query-able in the pool.
It's not guaranteed handle_incoming_txs will get called again after the node broadcasts the tx.
Even if handle_incoming_txs does get called once the node broadcasts the tx, we don't know if the tx was "just broadcasted" or if we're handling a tx we already knew about that we already pub'd via zmq.
It seems quite challenging to pass something into handle_incoming_tx on the initial submission to make it aware of the relay mechanism that will subsequently be used when the tx is ultimately relayed. Although it seems like it would be safe for "fluff" txs to be immediately visible in the db on initial submission during the "fluff" epoch (and to push them over zmq immediately), the way the relay code is structured, it would take a larger overhaul to safely implement that approach to avoid doing so for "stem" txs.

SChernykh · 2022-07-10T17:49:54Z

src/cryptonote_core/cryptonote_core.cpp

@@ -1406,21 +1406,69 @@ namespace cryptonote
    return true;
  }
  //-----------------------------------------------------------------------------------------------
+  bool core::notify_txpool_event(const epee::span<const cryptonote::blobdata> tx_blobs, const epee::span<const crypto::hash> tx_hashes, const epee::span<const cryptonote::transaction> txs, const std::vector<bool> &just_broadcasted) const
+  {


It should check that tx_hashes, txs, just_broadcasted have the same size as tx_blobs and return false if they don't match.

Also,

if (!m_zmq_pub) return true;

because this function is a no-op and can early out when there's no ZMQ pub.

Good defense, done

SChernykh · 2022-07-10T18:02:32Z

src/cryptonote_core/tx_pool.cpp

@@ -820,7 +820,7 @@ namespace cryptonote
    return true;
  }
  //---------------------------------------------------------------------------------
-  void tx_memory_pool::set_relayed(const epee::span<const crypto::hash> hashes, const relay_method method)
+  void tx_memory_pool::set_relayed(const epee::span<const crypto::hash> hashes, const relay_method method, std::vector<bool> &just_broadcasted)


It doesn't check that just_broadcasted is empty, so it can emplace_back in the wrong places in the loop.

if (!just_broadcasted.empty()) just_broadcasted.clear();

SChernykh · 2022-07-13T17:35:36Z

I ran it for 24 hours on my server, both p2pool nodes didn't complain about any duplicate transactions. I also tested it by sending a transaction through the same monerod - the tx was broadcasted to p2pool nodes via ZMQ 42.7 seconds later, as expected.

vtnerd · 2022-07-18T00:28:11Z

You commented on this - but to highlight to everyone -

The primary issue is that this leaks the origin (or re-broadcaster) to all connected ZMQ clients, but does not leak "normal" stem phase transactions. It looks like your counter argument was the difficulty of re-structuring the code to detect whether the local client was in the fluff epoch.

If this fix gets merged, I think an issue needs to be opened up about this because it is inconsistent (there's more privacy for non-origin txes).

vtnerd

Other than the blurb above about starting an issue, this patch looks fine.

vtnerd · 2022-07-18T00:10:51Z

src/cryptonote_core/cryptonote_core.cpp

+    bool valid_events = false;
+    for (std::size_t i = 0; i < results.size(); ++i)
+    {
+      results[i].tx = std::move(txs[i]);


You shouldn't be able to actually move these objects, its a const view of the transactions. I think this just results in the copy-constructor being called (which is annoying as the compiler doesn't actively complain).

Same thing on next line.

And indeed, this just results in a copy. I have such a love/hate for C++.

Ya this was odd of me to leave as is. Will fix.

This never changed, but I don't think its a big deal (other than being slightly deceptive). It probably makes sense just to leave it, because if the constness of the function ever changes this function call changes (although I think transasction does not have a proper move function).

vtnerd · 2022-07-18T00:29:39Z

src/cryptonote_core/tx_pool.cpp

  {
+    if (!just_broadcasted.empty())


This if is unnecessary.

What @vtnerd probably meant to say that just_broadcasted.clear(); can just be called without the if with the same end result.

Correct, the STL code has to do this check basically anyway internally.

vtnerd

Terrible brain fart. Ignore the above about creating a github issue. I was doing all of this from memory, on_transactions_relayed is the exact function you want as its only called in the levin notify code.

Sometimes its difficult for me to accept I wrote a bug :)

vtnerd · 2022-07-18T00:49:49Z

src/cryptonote_core/cryptonote_core.cpp

+
+    if (tx_blobs.size() != tx_hashes.size() || tx_blobs.size() != txs.size() || tx_blobs.size() != just_broadcasted.size())
+      return false;
+


I think you should add:

if (std::count(just_broadcasted.begin(), just_broadcasted.end(), std::size_t(0)) == 0) return true;

as another quick out optimization.

Perhaps there is still a misunderstanding, but if not, I think you meant a truthy 3rd param :) If the count of "just broadcasted" txs is 0, then we have nothing to publish. But if the count of txs that were not "just broadcasted" is 0, then there may still be some "just broadcasted" that we need to publish

I think the test should make it clear it should be truthy too; it hangs with a falsey 3rd param

if (std::count(just_broadcasted.begin(), just_broadcasted.end(), true) == 0) return true;

Right, I think you meant that^ @vtnerd

See the latest

Yes, I was doing this from memory quickly and forgot the API. Mine didn't make any sense.

SChernykh suggested changes Jul 10, 2022

View reviewed changes

j-berman force-pushed the zmq-fluff-submits branch from af7059f to 7ec74ab Compare July 10, 2022 20:33

SChernykh approved these changes Jul 13, 2022

View reviewed changes

vtnerd reviewed Jul 18, 2022

View reviewed changes

vtnerd approved these changes Jul 18, 2022

View reviewed changes

j-berman force-pushed the zmq-fluff-submits branch from 614fd83 to c367a11 Compare July 19, 2022 01:15

vtnerd approved these changes Jul 21, 2022

View reviewed changes

j-berman force-pushed the zmq-fluff-submits branch from c367a11 to fb94899 Compare July 21, 2022 18:13

Publish submitted txs via zmq

1fc60ca

j-berman force-pushed the zmq-fluff-submits branch from fb94899 to 1fc60ca Compare July 21, 2022 18:47

j-berman mentioned this pull request Jul 21, 2022

zmq: publish submitted txs via zmq [release-v0.18] #8451

Merged

selsta approved these changes Jul 22, 2022

View reviewed changes

Lissy93 approved these changes Aug 7, 2022

View reviewed changes

luigi1111 merged commit ce80747 into monero-project:master Aug 23, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

zmq: publish submitted txs via zmq #8427

zmq: publish submitted txs via zmq #8427

j-berman commented Jul 10, 2022

SChernykh Jul 10, 2022

SChernykh Jul 10, 2022

j-berman Jul 11, 2022

SChernykh Jul 10, 2022

j-berman Jul 11, 2022

SChernykh commented Jul 13, 2022

vtnerd commented Jul 18, 2022

vtnerd left a comment

vtnerd Jul 18, 2022

vtnerd Jul 18, 2022

j-berman Jul 18, 2022

vtnerd Jul 21, 2022 •

edited

Loading

vtnerd Jul 18, 2022

SChernykh Jul 18, 2022

vtnerd Jul 21, 2022

vtnerd left a comment

vtnerd Jul 18, 2022

j-berman Jul 19, 2022

SChernykh Jul 19, 2022

j-berman Jul 19, 2022

vtnerd Jul 21, 2022


		if (tx_blobs.size() != tx_hashes.size() \|\| tx_blobs.size() != txs.size() \|\| tx_blobs.size() != just_broadcasted.size())
		return false;

zmq: publish submitted txs via zmq #8427

zmq: publish submitted txs via zmq #8427

Conversation

j-berman commented Jul 10, 2022

Problem

Solution

Deeper context

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SChernykh commented Jul 13, 2022

vtnerd commented Jul 18, 2022

vtnerd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vtnerd Jul 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vtnerd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vtnerd Jul 21, 2022 •

edited

Loading