Page MenuHomePhabricator

Allow for messages to be up to 32MB
ClosedPublic

Authored by deadalnix on Jun 12 2017, 21:11.

Details

Summary

This ensure we can support bigger blocks, at least long enough so we'll see it comming.

I added test for blocks up to 16MB in the integration test to make sure this works. Without the message size limit, this doesn't work.

Test Plan
make check
../qa/pull-tester/rpc-tests.py abc-p2p-fullblocktest-simple.py

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

I would be hesitant to set the message limit to 32 MB if we are only testing up to 16MB block size. If we were able to successfully test up to 32M blocks I would be fine with setting the MAX_PROTOCOL_MESSAGE_LENGTH to 32MB. My view is we should increase it as we eliminate the obstacles and test it successfully at higher sizes.

What is the downside of keeping the MAX_PROTOCOL_MESSAGE_LENGTH tied to exactly the block size limit?

Also, do you already have an idea why it fails when trying 17MB ?
Setting MAX_PROTOCOL_MESSAGE_LENGTH to 16MB and testing up to 16MB seems to work alright.

The > 16MB might be something with the framework as I don't see an Oversize error at that size.

When setting to 32MB I get the Oversize error triggered in the client (and the test hangs due to this).

src/net.h
61 ↗(On Diff #412)

Consider setting this to 16MB which is max tested blocksize, and increasing it to 32MB only once we succeed with blocks up to that limit.

src/net.h
61 ↗(On Diff #412)

We don't want to be at capacity there.

What is the downside of keeping the MAX_PROTOCOL_MESSAGE_LENGTH tied to exactly the block size limit?

That limit is not available easily at that place in the code and making it available would be too big. The goal here is to have a nice margin that'll last long enough so we can actually do this properly. 32MB is fine and in fact was the limit for a long time.

Also, do you already have an idea why it fails when trying 17MB ?

Does it ? I set excessive block size on the node to be 16MB, but if put higher, I'd expect things to fail slightly before 32MB .

It worked for me up to 16MB but not 17.

I would be interested whether others can reproduce that.

So I think there is a chance that you actually do not have the nice margin you are hoping for.

src/net.h
61 ↗(On Diff #412)

For me it is a question of only allowing the software to run in a well-tested envelope where you can say it was verified to work (according to our current best efforts).

Outside of that envelope, one enters the realm of wishful thinking.

So I think there is a chance that you actually do not have the nice margin you are hoping for.

We have a nice margin on that issue. We'll need to push the envelope for others.

I opened T33 for the 17MB issue I found while pushing above what this test does.

In the meantime, I can accept this test.
Before official release (binaries) I would like to revisit the issue if there still is one at that time.

This revision is now accepted and ready to land.Jun 13 2017, 10:44
This revision was automatically updated to reflect the committed changes.