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

fluffy-blocks-p2p-bit-flags #1263

Merged

Conversation

@revler1082
Copy link

commented Oct 27, 2016

Pretty much same as #1242, but fixed up the p2p stuff so that it works with current release nodes. In the previous version I altered the existing handshake, etc structs to send extra data, so it didn't play nice with current nodes. The new one has an extra trip/call for support flags on handshake. If you run this against current release nodes you will see a bunch of errors, but it should be fine, it's just the "get_support_flags" command failing since release nodes don't have it.

@Gingeropolous

This comment has been minimized.

Copy link
Contributor

commented Oct 27, 2016

Hope I don't clutter this one up too much. You can yell at me if you'd like. I'm getting this interesting error, despite the fact that its overall working.

2016-Oct-26 23:29:22.161816 [P2P3]ERROR /home/ginger/thin2/contrib/epee/include/storages/portable_storage.h:153 portable_storage: wrong binary format, packet size = 0 less than expected sizeof(storage_block_header)=9
2016-Oct-26 23:29:22.162156 [P2P3]ERROR /home/ginger/thin2/contrib/epee/include/storages/levin_abstract_invoke2.h:129 Failed to load_from_binary on command 1007
2016-Oct-26 23:30:06.061173 [P2P2]ERROR /home/ginger/thin2/contrib/epee/include/storages/portable_storage.h:153 portable_storage: wrong binary format, packet size = 0 less than expected sizeof(storage_block_header)=9
2016-Oct-26 23:30:06.061202 [P2P2]ERROR /home/ginger/thin2/contrib/epee/include/storages/levin_abstract_invoke2.h:129 Failed to load_from_binary on command 1007

@revler1082

This comment has been minimized.

Copy link
Author

commented Oct 27, 2016

@Gingeropolous lol, totally fine, gotta identify the problems to deal with them. That error should be ok, command 1007 is a new command that I put in, but when it makes that request to existing clients who don't know about it, it fails. If you were to run your own private testnet composed of only fluffy nodes, you wouldn't see it.

@fluffypony

This comment has been minimized.

Copy link
Collaborator

commented Oct 29, 2016

Should we make this part of the January hard fork? Let's discuss it in tomorrow's meeting, @revler1082 will you be attending?

@revler1082

This comment has been minimized.

Copy link
Author

commented Oct 29, 2016

@fluffypony sure, #monero-dev on irc? Time? I'm doing stuff to my dog (only good things, lol) so probably from phone, hopefully won't have to type much.

@NanoAkron

This comment has been minimized.

Copy link
Contributor

commented Oct 29, 2016

I'll be running a fluffy blocks node soon if you start to see more fluffy traffic

I'd love to see any innovative stuff like this in a hard fork so the entire network remains compatible without needing the cruft of additional handshaking rules/support flags etc.

@olarks

This comment has been minimized.

Copy link

commented Oct 29, 2016

@revler1082 #monero-dev meeting is at 16:00 UTC tomorrow.

@moneromooo-monero
Copy link
Contributor

left a comment

Looks good in general, though random bits and bobs left to fix, like not checking error codes, and seemingly half done parts in the p2p code, which I don't know enough to know what's missing.
Also, please try not to change random whitespace.

{
if(parse_and_validate_tx_from_blob(tx_blob, tx))
{
get_transaction_hash(tx, tx_hash);

This comment has been minimized.

Copy link
@moneromooo-monero

moneromooo-monero Oct 30, 2016

Contributor

This can actually throw with rct (unlikely, but best guarding against it). Also return code checking.

if(!m_core.get_pool_transaction(tx_hash, tx))
{
cryptonote::tx_verification_context tvc = AUTO_VAL_INIT(tvc);
m_core.handle_incoming_tx(tx_blob, tvc, true, true);

This comment has been minimized.

Copy link
@moneromooo-monero

moneromooo-monero Oct 30, 2016

Contributor

Check return code

{
if(support_flags & P2P_SUPPORT_FLAG_FLUFFY_BLOCKS)
{
LOG_PRINT_YELLOW("PEER SUPPORTS FLUFFY BLOCKS - RELAYING THIN/COMPACT WHATEVER BLOCK", LOG_LEVEL_0);

This comment has been minimized.

Copy link
@moneromooo-monero

moneromooo-monero Oct 30, 2016

Contributor

Not level 0 on each block please

@@ -158,17 +158,18 @@ namespace nodetool
m_config.m_net_config.connection_timeout = P2P_DEFAULT_CONNECTION_TIMEOUT;
m_config.m_net_config.ping_connection_timeout = P2P_DEFAULT_PING_CONNECTION_TIMEOUT;
m_config.m_net_config.send_peerlist_sz = P2P_DEFAULT_PEERS_IN_HANDSHAKE;
m_config.m_support_flags = P2P_SUPPORT_FLAG_FLUFFY_BLOCKS; // | OTHER_FLAGS

This comment has been minimized.

Copy link
@moneromooo-monero

moneromooo-monero Oct 30, 2016

Contributor

Subjective, but I'd put a P2P_SUPPORT_FLAGS in cryptonote_config.h, initialized to that one flag, Makes it easier to find when adding another flag.

@@ -681,7 +683,7 @@ namespace nodetool
LOG_PRINT_CC_RED(context, "COMMAND_HANDSHAKE invoke failed. (" << code << ", " << epee::levin::get_err_descr(code) << ")", LOG_LEVEL_1);
return;
}

This comment has been minimized.

Copy link
@moneromooo-monero

moneromooo-monero Oct 30, 2016

Contributor

Adding random whitespace, eh ? :)

@@ -704,7 +706,8 @@ namespace nodetool
return;
}

pi = context.peer_id = rsp.node_data.peer_id;
pi = context.peer_id = rsp.node_data.peer_id;
//this->m_peerid_to_support_flags[pi] = 0; // assume they don't support anything

This comment has been minimized.

Copy link
@moneromooo-monero

moneromooo-monero Oct 30, 2016

Contributor

Looks like forgotten debug stuff

@@ -704,7 +706,8 @@ namespace nodetool
return;
}

pi = context.peer_id = rsp.node_data.peer_id;
pi = context.peer_id = rsp.node_data.peer_id;

This comment has been minimized.

Copy link
@moneromooo-monero

moneromooo-monero Oct 30, 2016

Contributor

No visible difference, just whitespace.

m_net_server.get_config_object(),
[=](int code, const typename COMMAND_REQUEST_SUPPORT_FLAGS::response& rsp, p2p_connection_context& context_)
{
//epee::misc_utils::auto_scope_leave_caller scope_exit_handler = epee::misc_utils::create_scope_leave_handler([&](){support_flags_event.raise();});

This comment has been minimized.

Copy link
@moneromooo-monero

moneromooo-monero Oct 30, 2016

Contributor

review this

f(context_, rsp.support_flags);
//context.support_flags = rsp.support_flags;
//this->m_peerid_to_support_flags[pi] = rsp.support_flags;
},

This comment has been minimized.

Copy link
@moneromooo-monero

moneromooo-monero Oct 30, 2016

Contributor

review this

//if(!r)
//{
// support_flags_event.wait();
//}

This comment has been minimized.

Copy link
@moneromooo-monero

moneromooo-monero Oct 30, 2016

Contributor

review this

@revler1082 revler1082 force-pushed the revler1082:fluffy-blocks-p2p-bit-flags branch from 9de89f2 to 8d2c632 Oct 30, 2016

@NanoAkron

This comment has been minimized.

Copy link
Contributor

commented Oct 31, 2016

Would it be worthwhile collecting live stats on space savings for fluffy vs standard blocks, or logging the number of transactions not in the local mempool that had to be fetched per block?

@revler1082

This comment has been minimized.

Copy link
Author

commented Oct 31, 2016

@NanoAkron would definitely be cool to log some real world metrics. It's pretty easy to model though, so maybe take 80% of the best case scenario as an estimate?

I think we can get some computational savings as well, since I'm 99% sure I can avoid verifying transactions again, which happens with the original relaying (but want moneromooo to verify).

@NanoAkron

This comment has been minimized.

Copy link
Contributor

commented Oct 31, 2016

Good point on computational savings.

@NanoAkron

This comment has been minimized.

Copy link
Contributor

commented Nov 1, 2016

Some questions for the expert - Am I right in thinking that we don't have the malleability issues of certain other coins (ahem)? Could this be used to determine the shortest transaction ID that is unlikely to result in a collision and gain further savings in terms of communication overheads? Only if the full transaction doesn't actually lead to a collision once received and compared to the contents of the local mempool (i.e. It's actually an unseen transaction) would the full transaction need to be requested matching the one with those initial bytes.

Or how about this - the requesting node tells the propagating node 'I have x transactions in my mempool whose first y bytes collide (where y is set by network policy), so please send the full versions if they're in this block, otherwise short IDs are OK'. Then the 'block' is just a series of short IDs to pick from the mempool and the nonce. The local node then reconstructs the block locally and verifies against the nonce before relaying onwards.

Or is that how this scheme works anyway... :)

@revler1082

This comment has been minimized.

Copy link
Author

commented Nov 1, 2016

Haha, I'm definitely no expert.

There are pros and cons with sending short transaction hashes. Yes, we'd save some space, but it increases the complexity of the whole scheme. Also, assuming the mempool data structure is some kind of hash table, we're turning a constant lookup into a not-so-efficient linear scan in order to match those prefixes. Of course all of this could be mitigated, but at the cost of increased code/time/complexity.

We have to balance the cost/benefit. If we can shrink data over the wire by 90% for x, it might not be worth going to 95% if it's going to cost 4x.

I did take an idea from @moneromooo-monero in the last meeting and have nodes ask for transactions by index instead of hash, so that should be another notable savings.

@NanoAkron

This comment has been minimized.

Copy link
Contributor

commented Nov 1, 2016

Cool. What's the punishment scheme for nodes that are in fact lying about the contents of the block so that when you try to reconstruct it locally from your mempool and validate it, it doesn't validate correctly?

I.e. They say they have a new block with nonce x and tx indices {1,2,3} but after you reconstruct locally it doesn't match. Is this a problem at your end or theirs and how do you punish malicious nodes sending fake blocks?

@revler1082

This comment has been minimized.

Copy link
Author

commented Nov 1, 2016

@NanoAkron validation is the same as with the non-fluffy block relaying. Nodes that send invalid transactions or blocks are dropped.

@NanoAkron

This comment has been minimized.

Copy link
Contributor

commented Nov 1, 2016

;) thanks for answering the silly questions.

@moneromooo-monero

This comment has been minimized.

Copy link
Contributor

commented Nov 1, 2016

There's a possible DoS attack though, where a node can claim a block, then, when asked for a set of txes in it, sends the block with missing txes. And again. And again, and again, and again. This will never get the node dropped. I'm not sure this is very significant, since it doesn't prevent a block from being received by another node. The possible vector is CPU usage, since the victim would have to revalidate the block all the time, as well as all N-1 txes (if just one is constantly missing).

@moneromooo-monero

This comment has been minimized.

Copy link
Contributor

commented Nov 1, 2016

Ah, that part about reverifying txes does not apply, since valid txes are kept in the pool. So probably fine to ignore.

@moneromooo-monero
Copy link
Contributor

left a comment

Some more comments, maybe the one about ordering is being paranoid, not sure.

Can you add something to only try fluffy blocks on testnet for now ? I'd rather it being run large(r) scale on testnet first soon.

// would be to pass these back/forth on the missing tx request,
// right now they're just being lost at the end of this function call
if(!tvc.m_added_to_pool)
tx_hash_to_blob[tx_hash] = tx_blob;

This comment has been minimized.

Copy link
@moneromooo-monero

moneromooo-monero Nov 1, 2016

Contributor

This will need some changes to the pool stuff, so these can be kept, at least temporarily, even if in the future we limit the pool. This is some future change though.

if(tx_hash_to_blob.find(tx_hash) != tx_hash_to_blob.end())
{
have_tx.push_back(tx_hash_to_blob[tx_hash]);
}

This comment has been minimized.

Copy link
@moneromooo-monero

moneromooo-monero Nov 1, 2016

Contributor

This is still wrong, since the next fluffy block call will not have that tx, so you will end up in a ping pong that never ends.

++tx_idx;
}

if(need_tx_indices.size() > 0) // drats, we don't have everything..

This comment has been minimized.

Copy link
@moneromooo-monero

moneromooo-monero Nov 1, 2016

Contributor

This is a std::list, so size is O(n). Use if (!need_tx_indices.empty()) instead, since we might get hundreds of txes at one point.

BOOST_FOREACH(auto& tx_idx, need_tx_indices)
{
missing_tx_req.missing_tx_indices.push_back(tx_idx);
}

This comment has been minimized.

Copy link
@moneromooo-monero

moneromooo-monero Nov 1, 2016

Contributor

If you make them the same type in the first place, you can just copy or std::move the object at once.

if(tx_idx < local_txs_count)
{
fluffy_response.b.txs.push_back(t_serializable_object_to_blob( *(std::next(local_txs.begin(), tx_idx)) ));
}

This comment has been minimized.

Copy link
@moneromooo-monero

moneromooo-monero Nov 1, 2016

Contributor

I'm unconvinced we're getting an ordering constraint here. Looking at the code, it seems we do, but I'd be more comfortable calling m_core.get_block_id_by_height(arg.current_blockchain_height - 1) + get_block_by_hash, above, then going through txes. I'm open to counterargument though. Like if some other CN code already relies on ordering, it's OK. It just feels like something that may bite us in the rear later.

This comment has been minimized.

Copy link
@revler1082

revler1082 Nov 3, 2016

Author

Traced the transaction fill code to -> get_transactions(blk.tx_hashes, txs, missed_ids). The get_transaction function then goes thru the tx_hashes in order and pushes onto txs. So assuming, the blk.tx_hashes is in order, then the txs should be in order?

@moneromooo-monero

This comment has been minimized.

Copy link
Contributor

commented Nov 3, 2016

ok. Again, a few minor comments, but only if you're not sick of it yet :P
Thanks, great patch :)

*
* @return are we on testnet?
*/
bool get_testnet() { return m_testnet; };

This comment has been minimized.

Copy link
@moneromooo-monero
@@ -69,6 +69,8 @@ namespace cryptonote

uint64_t avg_upload;
uint64_t current_upload;

uint32_t support_flags;

This comment has been minimized.

Copy link
@moneromooo-monero

moneromooo-monero Nov 3, 2016

Contributor

I feel pedantic, but... can you keep the same indent as the previous fields please :)

// If requested objects is not empty, then we must have asked for
// some missing transacionts, make sure that they're all there.
//
// Can I safely re-use this field? I think so, but someone check me!

This comment has been minimized.

Copy link
@moneromooo-monero

moneromooo-monero Nov 3, 2016

Contributor

I don't know. Someone would need to dive in the p2p code to see what happens when we get messages we do not expect (ie, a sync response when we did not send a sync request). I think it's OK, but I'm not sure :)

{
if(m_core.get_testnet() && support_flags & P2P_SUPPORT_FLAG_FLUFFY_BLOCKS)
{
LOG_PRINT_YELLOW("PEER SUPPORTS FLUFFY BLOCKS - RELAYING THIN/COMPACT WHATEVER BLOCK", LOG_LEVEL_0);

This comment has been minimized.

Copy link
@moneromooo-monero

moneromooo-monero Nov 3, 2016

Contributor

It was level 2 last time :)

This comment has been minimized.

Copy link
@revler1082

revler1082 Nov 3, 2016

Author

lol, yea I keep changing it when I'm messing around, just like the do_not_relay on wallet2, but I always forget to set this back!

This comment has been minimized.

Copy link
@NanoAkron

NanoAkron Nov 3, 2016

Contributor

Although surely if people are running this on test net they might want to know who is supporting fluffy and who isn't at level 0? But it agree that in the production version this should be at a higher log level.

This comment has been minimized.

Copy link
@moneromooo-monero

moneromooo-monero Nov 3, 2016

Contributor

Level 1 would be a good middle ground maybe :)

@revler1082

This comment has been minimized.

Copy link
Author

commented Nov 3, 2016

@moneromooo-monero thanks for all of your help, never feel bad about pointing anything and everything out. I want this to be as robust as possible, and it'll only get that way if everyone looks it over super close.

@revler1082 revler1082 force-pushed the revler1082:fluffy-blocks-p2p-bit-flags branch from 7307d24 to 5701d46 Nov 4, 2016

@moneromooo-monero

This comment has been minimized.

Copy link
Contributor

commented Nov 4, 2016

Thanks, I think it's all fine now.
Can you squash those patches btw ? The reason I have to avoid random whitespace changes is to avoid git log spam, and it's kinda defeated by having a patch adding that and another removing it :)
In the future, when you have multiple "logical" patches in a PR, it's a lot easier to squash/rebase as you progress (or at least I find it a lot easier, even if it's extra work, as it pays off later).
But yeah, thanks for the patch :)

@revler1082 revler1082 force-pushed the revler1082:fluffy-blocks-p2p-bit-flags branch from 5701d46 to 584439e Nov 4, 2016

@moneromooo-monero

This comment has been minimized.

Copy link
Contributor

commented Nov 5, 2016

Not sure what happened there, looks like two commits, broken up in weird ways. Can you also have each logical commit have an actual first line description (ie, not "This is a combination of 11 commits."), and there's no need to mention the "fix previous error", since the whole point of squashing is to "undo" them in the first place so they are not in git at all.

@revler1082 revler1082 force-pushed the revler1082:fluffy-blocks-p2p-bit-flags branch from 584439e to e23dfe3 Nov 9, 2016

@revler1082

This comment has been minimized.

Copy link
Author

commented Nov 9, 2016

@moneromooo-monero as always, apologies for my poopy git skills. I've tried it again, let me know if that's good :)

@moneromooo-monero

This comment has been minimized.

Copy link
Contributor

commented Nov 9, 2016

Pretty much! Just one thing left, and I apologize again for being so annoying:
git commit --amend
Add this as first line: add lightweight block propagation ("fluffy blocks")
First line appears alone in short logs, so it's really helpful to have an overview, with the rest of the commit message being details :)

Thank you very much

Dion Ahmetaj Dion Ahmetaj
add lightweight block propagation ("fluffy blocks")
Added a new command to the P2P protocol definitions to allow querying for support flags.

Implemented handling of new support flags command in net_node. Changed for_each callback template to include support flags. Updated print_connections command to show peer support flags.

Added p2p constant for signaling fluffy block support.

Added get_pool_transaction function to cryptnote_core.

Added new commands to cryptonote protocol for relaying fluffy blocks.

Implemented handling of fluffy block command in cryptonote protocol.

Enabled fluffy block support in node initial configuration.

Implemented get_testnet function in cryptonote_core.

Made it so that fluffy blocks only run on testnet.

@revler1082 revler1082 force-pushed the revler1082:fluffy-blocks-p2p-bit-flags branch from e23dfe3 to d61bd81 Nov 10, 2016

@moneromooo-monero

This comment has been minimized.

Copy link
Contributor

commented Nov 10, 2016

Wicked, thanks!

@fluffypony
Copy link
Collaborator

left a comment

Reviewed

@fluffypony fluffypony merged commit d61bd81 into monero-project:master Nov 11, 2016

7 of 10 checks passed

buildbot/monero-static-debian-armv8 Build done.
Details
buildbot/monero-static-ubuntu-amd64 Build done.
Details
buildbot/monero-static-ubuntu-i686 Build done.
Details
buildbot/monero-static-freebsd64 Build done.
Details
buildbot/monero-static-osx-10.10 Build done.
Details
buildbot/monero-static-osx-10.11 Build done.
Details
buildbot/monero-static-osx-10.12 Build done.
Details
buildbot/monero-static-ubuntu-arm7 Build done.
Details
buildbot/monero-static-win32 Build done.
Details
buildbot/monero-static-win64 Build done.
Details
fluffypony added a commit that referenced this pull request Nov 11, 2016
Merge pull request #1263
d61bd81 add lightweight block propagation ("fluffy blocks") (Dion Ahmetaj)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.