Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

protocol: request txpool contents when synced #6214

Merged
merged 1 commit into from Mar 31, 2020

Conversation

@moneromooo-monero
Copy link
Collaborator

moneromooo-monero commented Dec 4, 2019

A newly synced Alice sends a (typically quite small) list of
txids in the local tpxool to a random peer Bob, who then uses
the existing tx relay system to send Alice any tx in his txpool
which is not in the list Alice sent

}
return true;
}, false);
return true;

This comment has been minimized.

Copy link
@stoffu

stoffu Dec 5, 2019

Contributor

Isn't it better to return false to indicate failure/error for these MERROR cases than silently ignoring them?

This comment has been minimized.

Copy link
@moneromooo-monero

moneromooo-monero Dec 5, 2019

Author Collaborator

The inner true means "continue anyway". It seemed better to just skip the erroring ones, if any, and still return what you can.

@stoffu
stoffu approved these changes Dec 6, 2019
@moneromooo-monero moneromooo-monero force-pushed the moneromooo-monero:txpc branch from 12db141 to 56bb742 Dec 13, 2019
@Snipa22

This comment has been minimized.

Copy link
Collaborator

Snipa22 commented Mar 12, 2020

@moneromooo-monero This MR has conflicts that need to be resolved.

@moneromooo-monero moneromooo-monero force-pushed the moneromooo-monero:txpc branch from 56bb742 to 5c6c21b Mar 12, 2020
@moneromooo-monero

This comment has been minimized.

Copy link
Collaborator Author

moneromooo-monero commented Mar 12, 2020

fixed

CRITICAL_REGION_LOCAL1(m_blockchain);

m_blockchain.for_all_txpool_txes([this, &hashes, &txes](const crypto::hash &txid, const txpool_tx_meta_t &meta, const cryptonote::blobdata*) {
const auto i = std::find(hashes.begin(), hashes.end(), txid);

This comment has been minimized.

Copy link
@vtnerd

vtnerd Mar 12, 2020

Contributor

Crap I missed this review, but this can be done immediately after the PR. This should be sorted and then std::lower_bound somewhere. Otherwise, the peer can jam a long list of bogus hashes and DoS easier.

This comment has been minimized.

Copy link
@vtnerd

vtnerd Mar 12, 2020

Contributor

Or no wait, sorting is a n*logn operation, and this is only searching by a smaller amount in the txpool. So hmm. The stuff in txpool is probably already sorted by hash too, which would accelerate the search. But I think that's part of internal DB details.

{
if (!m_blockchain.get_txpool_tx_blob(txid, bd, cryptonote::relay_category::broadcasted))
{
MERROR("Failed to get blob for txpool transaction " << txid);

This comment has been minimized.

Copy link
@vtnerd

vtnerd Mar 12, 2020

Contributor

This error message should display on stempool/local txes, which may not be useful.

This comment has been minimized.

Copy link
@vtnerd

vtnerd Mar 12, 2020

Contributor

Although, leave it maybe? Because its rarely going to happen in those other cases I think.

@moneromooo-monero

This comment has been minimized.

Copy link
Collaborator Author

moneromooo-monero commented Mar 12, 2020

The dandelion stuff changes this indeed. I think this is appropriate. Agreed ? The relay_method enum does not have a dandelion enum, I assume it's just when !fluff && !block && !local ?

diff --git a/src/cryptonote_core/tx_pool.cpp b/src/cryptonote_core/tx_pool.cpp
index 89fa30a03..73bf70647 100644
--- a/src/cryptonote_core/tx_pool.cpp
+++ b/src/cryptonote_core/tx_pool.cpp
@@ -600,6 +600,9 @@ namespace cryptonote
     CRITICAL_REGION_LOCAL1(m_blockchain);
 
     m_blockchain.for_all_txpool_txes([this, &hashes, &txes](const crypto::hash &txid, const txpool_tx_meta_t &meta, const cryptonote::blobdata*) {
+      const auto relay_method = meta.get_relay_method();
+      if (relay_method != cryptonote::block && relay_method != cryptonote::fluff)
+        return true;
       const auto i = std::find(hashes.begin(), hashes.end(), txid);
       if (i == hashes.end())
       {

@moneromooo-monero moneromooo-monero force-pushed the moneromooo-monero:txpc branch from 5c6c21b to 79dfd26 Mar 19, 2020
@moneromooo-monero

This comment has been minimized.

Copy link
Collaborator Author

moneromooo-monero commented Mar 19, 2020

I added it then.

@selsta

This comment has been minimized.

Copy link
Contributor

selsta commented Mar 20, 2020

Compilation failed.

[ 31%] Building CXX object src/cryptonote_core/CMakeFiles/obj_cryptonote_core.dir/tx_pool.cpp.o
/Users/runner/runners/2.165.2/work/monero/monero/src/cryptonote_core/tx_pool.cpp:604:45: error: expected '(' for function-style cast or type construction
      if (relay_method != cryptonote::block && relay_method != cryptonote::fluff)
                          ~~~~~~~~~~~~~~~~~ ^
/Users/runner/runners/2.165.2/work/monero/monero/src/cryptonote_core/tx_pool.cpp:604:76: error: no member named 'fluff' in namespace 'cryptonote'
      if (relay_method != cryptonote::block && relay_method != cryptonote::fluff)
                                                               ~~~~~~~~~~~~^
In file included from /Users/runner/runners/2.165.2/work/monero/monero/src/cryptonote_core/tx_pool.cpp:36:
@vtnerd

This comment has been minimized.

Copy link
Contributor

vtnerd commented Mar 21, 2020

Yeah this is a enum class so its namespaced in cryptonote::relay_method

A newly synced Alice sends a (typically quite small) list of
txids in the local tpxool to a random peer Bob, who then uses
the existing tx relay system to send Alice any tx in his txpool
which is not in the list Alice sent
@moneromooo-monero moneromooo-monero force-pushed the moneromooo-monero:txpc branch from 79dfd26 to 054b4c7 Mar 22, 2020
@moneromooo-monero

This comment has been minimized.

Copy link
Collaborator Author

moneromooo-monero commented Mar 22, 2020

fixed

@luigi1111 luigi1111 merged commit 096e213 into monero-project:master Mar 31, 2020
6 checks passed
6 checks passed
build-macos
Details
build-windows
Details
build-ubuntu
Details
libwallet-ubuntu
Details
test-ubuntu
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.