-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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 #1242
Fluffy blocks #1242
Conversation
This breaks building the core_proxy tests |
Please squash, and maybe describe more fully what this intends to achieve? A whitepaper would be helpful. |
@danrmiller thanks for the heads up, I'll look into that. Good idea @NanoAkron, I was trying to achieve something like what is described by fluffy here --> http://monero.stackexchange.com/a/1268/102 |
Ah, so something like XThin or Compact Blocks. I didn't see any part of your code where you build a bloom filter so will this actually work as intended? |
Neat! @NanoAkron bloom filtering opens up a DoS attack, so that's undesirable. @revler1082 great work; perhaps it's worth your while reading through the BIP 152 document, as that details how compact blocks work. We don't have to implement it exactly the same way, but details on the short transaction IDs (and so on) are immensely useful. |
That seems pretty nice. See comments inside for things I think don't work, though it may well be I got the wrong end of the stick, so an explanation instead would be nice. Thanks! |
int t_cryptonote_protocol_handler<t_core>::handle_notify_new_fluffy_block(int command, NOTIFY_NEW_FLUFFY_BLOCK::request& arg, cryptonote_connection_context& context) | ||
{ | ||
LOG_PRINT_CCONTEXT_L0("NOTIFY_NEW_FLUFFY_BLOCK (hop " << arg.hop << ")"); | ||
if(context.m_state == cryptonote_connection_context::state_normal) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Subjective, but I'd rather having it as: if (context.m_state != cryptonote_connection_context::state_normal) return 1; as the existing one already does, to avoid mountains of indentation below.
); | ||
|
||
m_core.resume_mine(); | ||
m_p2p->drop_connection(context); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd add a return here for safety if more code gets added at the epilogue later
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, I'll change things around to conform to the already existing coding style (gotta start following that contributor's guide) :)
crypto::hash tx_hash; | ||
std::unordered_map<crypto::hash, blobdata> tx_hash_to_blob; | ||
BOOST_FOREACH(auto& tx_blob, arg.b.txs) | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is wrong. prepare_handle_incoming_blocks was not called before handle_incoming_tx a few lines below. These txes seem optional, so testing might not have seen that since it wouldn't be used, but a peer may decide to send a tx here, since block_complete_entry has a tx list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, my main goal here was to get the transactions into the pool if they weren't already, so I definitely need to add a check to see if it's already in there and avoid doing all the extra work.
Is there a better way to verify and add to pool without using the handle_incoming_tx? I'll have a look later.
Worth adding a check that the transactions included are part of the tx_hashes, otherwise drop peer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not very familiar with this code, I was comparing to the existing, whereprepare_handle_incoming_blocks is called before adding the transactions. Is there a particular reason you don't want to call it ?
{ | ||
if(tx_hash_to_blob.find(tx_hash) != tx_hash_to_blob.end()) | ||
{ | ||
have_tx.push_back(tx_hash_to_blob[tx_hash]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't get this bit. Can you elaborate ?
The transactions are checked whether we have them already in the pool. If it is not in the pool, this means it might have been received in the optional tx set in that new P2P call. However, it's only added if it was not added to the pool, which means it got rejected (if it conflicts with a tx already in the pool, it will be added since kept_from_block is true). Therefore, I do not see the utility of that tx_hash_to_blob (and in fact it might make things worse, if we can succeed adding a block with a bad tx in it, though I'm not sure it can happen).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the only way it wouldn't be added to the pool is if it's a bad transaction?
If it was rejected wouldn't it fail the tvc.m_verifivation_failed check earlier?
I'm almost certain I added that because I was getting tripped up by transactions passing the check, but not being added to the pool. Could have something to do with me setting do_not_relay to true in wallet2.cpp for testing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the only reason it would not be added is it's bad. However, in the future, it could plausible also mean "no space left", so it's not a good thing to assume.
If it was rejected, it'd get the m_verifivation_failed flag to false, yes.
do_not_relay will not prevent transactions from being added to the pool.
Maybe I just don't understand how that part works and need to re-read. I'll comment again when I have done so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have read again.
First, I get your logic now. I thought the "added to pool" would be false if the tx was already there, but that's not the case. So that code makes sense in theory.
Second, in the case where you get a transaction T that does not get added to the pool (which will be because it's wrong somehow, but we should not rely on that for the future), then you keep its hash and blob,and will end up placing this tx in have_tx, rather than need_tx_hashes. If need_tx_hashes ends up non empty, this local tx cache gets wiped, and you won't have it anywhere when you get the missing txes back after calling a NOTIFY_REQUEST_FLUFFY_MISSING_TX. This means the receive part will fail, since that transaction T is not in the pool, but also not in the cache anymore, since you're long gone from that function.
So I think this local cache is pointless, and actually breaks things.
blocks.push_back(b); | ||
m_core.prepare_handle_incoming_blocks(blocks); | ||
|
||
for(auto tx_blob_it = arg.b.txs.begin(); tx_blob_it!=arg.b.txs.end();tx_blob_it++) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems wrong. It's going through the optional list of txes (likely empty), instead of the one you built.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whooops, that looks like a big one. Thanks a bunch!!
{ | ||
LOG_ERROR_CCONTEXT("failed to handle request NOTIFY_REQUEST_FLUFFY_MISSING_TX, dropping connection"); | ||
m_p2p->drop_connection(context); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return
@@ -774,10 +963,17 @@ namespace cryptonote | |||
template<class t_core> | |||
bool t_cryptonote_protocol_handler<t_core>::relay_block(NOTIFY_NEW_BLOCK::request& arg, cryptonote_connection_context& exclude_context) | |||
{ | |||
// |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stray chunk
@@ -783,7 +783,7 @@ namespace cryptonote | |||
if(bvc.m_added_to_main_chain) | |||
{ | |||
cryptonote_connection_context exclude_context = boost::value_initialized<cryptonote_connection_context>(); | |||
NOTIFY_NEW_BLOCK::request arg = AUTO_VAL_INIT(arg); | |||
NOTIFY_NEW_FLUFFY_BLOCK::request arg = AUTO_VAL_INIT(arg); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That will partition the network. I don't think there's a P2P version broadcast on connect, but if not one could be added, and use the new fluffy types only when the peer is recent enough. Partitioning in this way would be very harsh to the network I think, since the "good" chain would have ~0 hash rate for a good while.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's another big one! Thanks again for taking the time to go through everything. Much appreciated.
@@ -103,7 +103,6 @@ namespace cryptonote | |||
END_KV_SERIALIZE_MAP() | |||
}; | |||
|
|||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stray
Sweet! One minor thing, could we please use a more descriptive name than "fluffy block"? |
} | ||
|
||
post_notify<NOTIFY_REQUEST_FLUFFY_MISSING_TX>(missing_tx_req, context); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is missing m_core.resume_mine();
I added a couple more comments that need fixing, but once that's done it's a very nice change :) |
I agree - introducing this stuff now gives us good room for growth in the future. I also kinda like the name 'fluffy blocks' - we can always rename later. However, after recent presentations regarding the witnessed benefits of XThin blocks, I would like to hear more about why we are planning on implementing the compact block protocol instead. https://speakerdeck.com/dagurval/xthin-vs-compact-blocks https://www.youtube.com/watch?v=zkwNVpZYYng&feature=youtu.be Could we perhaps adopt XT/BU's 'Expedited' block transport mechanism as well? |
@NanoAkron thanks for the links. I'm definitely going to look deeper into those individual implementations. Honestly, I was never well versed in either, I just knew of the high level goal and that's what I tried to develop here. Thanks for the name +1, but everyone feel free to change it / vote for something better. Thank you all again for your comments and feedback, they've been extremely helpful. |
@NanoAkron @revler1082 I think let's stick to Compact Blocks for now, XThin isn't massively well-specced, and there are other issues with it. Here's a good write-up where @gmaxwell responds to that presentation. Also, this is security software - simpler is better. All we're really trying to accomplish is for a node to say "here's the header, what transactions do you need?" - no bloom filter, no complexity. |
I tried building and running your branch. Monerod couldn't exit - mutliple exit commands given. Managed to grab a bt (gdb) bt No idea if this is specific to the new stuff in your branch or where your branch stands in relation to head. |
@Gingeropolous hmm, I can't think of anything obvious off the top of my head that would mess with the exit, but I have to be better about testing in more environments in general. I'm working on win64, and the exit command works. |
Hi all, Just to document some extra information here - I've received a response from one of the Bitcoin Unlimited devs via Reddit, who pointed me towards https://github.com/BitcoinUnlimited/BitcoinUnlimited/blob/0.12.1bu/doc/bu-xthin-protocol.md as the documentation for the XThin implementation. Purely FYI, no agenda. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new stuff looks ok at a quick glance. Try to get into the habit of having clean patches while you work, it makes it a lot less painful than to have to rebase/squash everything just at the end, where everything invariably conflicts. I'd suggest a first patch to add protocol version, then a second to add fluffy blocks.
How to do protocol version is worth an IRC discussion too.
* | ||
* @note see tx_memory_pool::get_transaction | ||
*/ | ||
bool get_pool_transaction(const crypto::hash& id, transaction& tx) const; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That looks suspicious. I added that yesterday...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I probably need to rebase with a more recent version of master?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, always rebase if you can
{ | ||
if(protocol_version >= FLUFFY_MIN_PROTOCOL_VERSION) | ||
{ | ||
LOG_PRINT_YELLOW("PEER SUPPORTS FLUFFY",LOG_LEVEL_0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Level 2 please :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aint gonna see nothing with all the other stuff at level 2!!! But I get it.
|
||
// Do I need to do this? Every transaction is either already in the pool | ||
// and thus verified(?), or I just verified it above if it was included | ||
// in the sent txs. So is this a double check / wasteful? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@moneromooo-monero could you double check me on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just had a quick look earlier, I will review better when I get some time
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
XThin does a preliminary verification before relaying, followed by a detailed local verification afterwards. Now quite what that means, I don't know, but that's what they describe in their process.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess you could always ask before each send, although there is overhead in that and I personally think the swap capabilities on handshake model is better.
fluffypony suggested using bitflags for the version, so a node can claim support for this particular thing but not that one. I guess that could help if separate implementations pop up one day. Also, these bits aren't long term, so they can be reused over time since the network doesn't need to support really old nodes forever. |
Could this be rolled into the hard fork schedule? i.e. optional at V4, compulsory at V5? |
Version bits it is, that's a great idea. @NanoAkron that would make my year, lol, but it's much better if it's uber thoroughly tested / reviewed. There's not much of a rush right now since the transactions per block are so low, but in the future WHEN Monero is doing all of the worlds cash transactions, should be a good savings. |
…lock to newer peers. fixed some bugs pointed out by moneromoo.
…_proxy failure, but my fix is most likely not optimal/correct.
I'm still getting weird behavior. Here's a set_log 2 for everything after my node seems to have stopped getting data from the network (stuck at one block - 1164687, but thinks its 100%). http://nodebox.link/thinblocks_err.txt Maybe my data.mdb is messed up? Well, I'll try synchronizing from scratch. The context - had a Warptangent node going fine, then i swapped in this newly compiled monerod and started things up. It looks like its working until it just stops doing stuff. Indeed, the set_log 2 is empty, yet the monerod process is using 100% of CPU. Exit command fails. Have to kill 9. Edited to add - discovered my system clock was way off for some reason... could be an unrelated bug. Classic gingeropolous. Carry on. |
@Gingeropolous apologies if I'm reading this wrong, but were you trying to run this while connected to real-world mainnet waprtangent nodes? I'm not sure why it would freak out even if you compiled an older commit since you very likely didn't find/transmit a block so never hit the fluffy code. I'm running the latest build on real-world mainnet just to see if I can reproduce, currently seems to be syncing ok, but I'll update you when I'm fully synced. |
Yeah, I'm on mainnet. I edited my comment to indicate that my system clock was borked somehow, and maybe that was causing me to get blocked from the network. I'm testing again. I'm still experiencing the strange exit behavior. I'm on Ubuntu 16. Is this the fluffy_blocks specific log |
No, NOTIFY_NEW_TRANSACTIONS is part of the original, the new calls I added include the word "FLUFFY". There have been some other updates to master that get included as I rebase, maybe something in there? I'm in poopy internet area so syncing is taking forever, but I'm using my existing database. Not relevant, but made me laugh --> http://imgur.com/RZM9xhC |
add me as a priority node! This box should be fluffy. 104.168.99.235. Oh son of a biscuit. Apparently I wasn't building the branch. Argh! |
88a0fe1
to
3cef650
Compare
Yea the version bit stuff is the new new stuff, I just pushed it so you can grab it and compile that if you'd like (apologies if I blow up your computer, lol). |
are you on IRC or slack? should I use branch fluffy-blocks or fluffy-blocks2? |
they should be the same right now, fluffy-blocks is what i'd always use, I just made the fluffy-blocks-2 branch so I could make sure my stuff doesn't look crazy before pushing here since my git skillz are very not-1337, lol. I gotta start getting on the slack/irc stuff, but I think i'm gonna be out soon, have to wake up early for work tomorrow, but I'll leave my box on and add you as a node, seems like a pretty cool real-world test. |
@Gingeropolous haha, we have liftoff --> http://imgur.com/0lhdZJy |
Yeah, even on that VPS i got the same 100% cpu thing with the blockchain stuck at some random height. 1164895 - what did your node do at that height? Should also note that my build failed core tests, but monerod got built. |
I'm currently at 1164913 and running fine, no performance issues. All nodes are normal so just relaying full blocks. |
hrm, I saw this in my logs. 2016-Oct-24 22:28:42.789097 [P2P4]ERROR /home/ginger/thin2/contrib/epee/include/serialization/keyvalue_serialization_overloads.h:153 size in blob 1608 not have not zero modulo for sizeof(value_type) = 28 probably not related. So something's wrong with my compiling then if you're running fine. Because my home box just borked too. |
So this is where my build is failing: [ 80%] Building CXX object tests/core_proxy/CMakeFiles/core_proxy.dir/core_proxy.cpp.o |
Hahahha that's just hilarious, fluffy block. On Oct 22, 2016 14:41, "Lars Andersen" notifications@github.com wrote:
|
I'm going to close this since it's gotten a bit messy and open a neater PR. Thank you to everyone for your help and feedback. Also @Gingeropolous, you were right, the p2p code updates were bad and causing the handshake to fail under certain circumstances. |
Any feedback and testing help would be greatly appreciated. Am I going about this all wrong?