search

dashpay / dash

Dash - Reinventing Cryptocurrency
https://www.dash.org
MIT License
1.49k stars 1.2k forks source link

ProcessGetData, dealing with blocks + txs in batch - loop issue #3551

Open sidhujag opened 4 years ago

sidhujag commented 4 years ago

https://github.com/dashpay/dash/blob/master/src/net_processing.cpp#L1278 this should probably be continue and not break.. if you get multiple getdata requests some block some txs you still want to process the txs.

UdjinM6 commented 4 years ago

That's by design. Processing getdata for blocks can be quite expensive, so we only do this once per call. It works like this: process all known invs while it's not (some kind of) a block, process that block, drop all processed invs from vRecvGetData https://github.com/dashpay/dash/blob/master/src/net_processing.cpp#L1433. The rest of vRecvGetData is going to be processed on another call, up to another block, and so on.

sidhujag commented 4 years ago

it will break right now soon as it gets block so it will process maybe some INV's not blocks and soon as it gets to a block it will stop and continue on next loop.. is this to optimize tx INV's over blocks?

UdjinM6 commented 4 years ago

As far as I understand it It's basically to avoid processing multiple heavy operations in a row - many other objects are cached and/or small, blocks on the other hand can be few MiBs large, require disk i/o and might also require chain activation. So in order to keep our communication with peers live we have to process data in batches here.

sidhujag commented 4 years ago

ok cool, maybe it should be made more clear or better there is a way to make it more consistent with upstream bitcoin by not even going in there if your a block you can see how I did it here: https://github.com/syscoin/syscoin/blob/dev-4.x-evo/src/net_processing.cpp#L1782

Side note: you also have to deal with m_tx_relay being nullptr meaning block only relay mode. Because BTC assumes if its not a block its a tx, you need to handle the case where its not block or tx but MN types, for this I made IsMnType() and you can see where I used it.

PastaPastaPasta commented 4 years ago

I haven't looked into it, but I'm going to guess that is a bitcoin PR that does what your describing that we haven't backported yet.

sidhujag commented 4 years ago

The tx_relay thing maybe, but might be better design to check mntype upfront and where notfound is referenced as it is assumed in bitcoin that if its not a block its a tx but in our case we have other types to deal with.

In general its probably a cleaner logic flow