Page MenuHomePhabricator

Implement "EBx.y" indicator on subversion (user agent)
ClosedPublic

Authored by sickpig on Jun 19 2017, 07:37.

Details

Summary

Add excessiveBlockSize indicator in the subversion string in MB.
The value would be truncated to the first decimal digit.

Following two example of what the subver will be in case of
different EB settings:

  1. excessiveBlockSize set to 2000000 --> /Bitcoin ABC:0.14.1(EB2)/
  1. excessiveBlockSize set to 1560000 --> /Bitcoin ABC:0.14.1(EB1.5)/

Add also functional tests for subver EB signalling to abc-{cmdline,rpc}.py

Test Plan

run abc-{cmdline,rpc}.py

Diff Detail

Repository
rABC Bitcoin ABC
Branch
new/add-EB-to-subver
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 358
Build 358: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Ok, indeed UA uses parentheses on other clients, i was confused with the coinbase string. forget about that comment.

sickpig edited edge metadata.

Fix comment in functional tests
Remove global var strSubVersion, use userAgent() instead to get an up-to-date user agent string
Add unit test for getSubVersionEB

src/net.cpp
2787 ↗(On Diff #550)

Pass the config down.

2810 ↗(On Diff #550)

It doesn't looks like this belongs here.

src/net.h
807 ↗(On Diff #550)

Why is this public ?

src/rpc/abc.cpp
8 ↗(On Diff #550)

There is no reason to create that dependency. In fact, there is probably no reason to change this file at all.

src/util.h
18 ↗(On Diff #550)

???

deadalnix requested changes to this revision.Jun 19 2017, 13:50
This revision now requires changes to proceed.Jun 19 2017, 13:50
sickpig edited edge metadata.

Formatting of src/test/net_tests.cpp and src/qt/clientmodel.cpp

sickpig added inline comments.
src/net.cpp
2787 ↗(On Diff #550)

Ok I'll try

2810 ↗(On Diff #550)

what? the check on string length?

src/net.h
807 ↗(On Diff #550)

to unit test, but I suppose there's to be a way to unit test also private function

src/rpc/abc.cpp
8 ↗(On Diff #550)

it's a left over, need to be removed.

sickpig added inline comments.
src/util.h
18 ↗(On Diff #550)

another left over

src/net.cpp
2810 ↗(On Diff #550)

do you want me to extract the len check from userAgent()?

doing that would make the code a little bit cumbersome cause every time we call the function we have to check for its res len and shrink it if to long...

Pass config to userAgent()
Remove useless header inclusion from ab.cpp and util.h
TODO: find a better place for userAgent() (?), fix getSubVerEB() arithmetic properly

deadalnix requested changes to this revision.Jun 19 2017, 15:46
deadalnix added inline comments.
src/net.cpp
26 ↗(On Diff #555)

Why ?

src/net.h
807 ↗(On Diff #550)

You should unittest userAgent, getSubVersionEB is used nowhere, so i'm not sure what the point is.

src/rpc/abc.cpp
8 ↗(On Diff #555)

Still need to be removed.

This revision now requires changes to proceed.Jun 19 2017, 15:46
sickpig added inline comments.
src/net.cpp
26 ↗(On Diff #555)

to use std::fesetround(FE_DOWNWARD) and std::fesetround(FE_TONEAREST);
this is the kludge that need to go mentioned in my last comment

src/net.h
807 ↗(On Diff #550)

userAgent() is tested already in the functional test contained in abc-cmdline.py and abc-rpc.py, freetrader asked for a unit test for getSubVerEB and I added it.

And such test help me find it a round bug. In any case you are right, I'm going to move what I'm testing in src/test/net_tests.cpp to abc-rpc.py

src/rpc/abc.cpp
8 ↗(On Diff #555)

I could swear I did it... never mind I'll remove it for good in the next iteration

src/net.cpp
26 ↗(On Diff #555)

We shouldn't be using floating point at all for anything consensus related.

Also, presumably, this should be removed from somewhere else.

src/net.h
807 ↗(On Diff #550)

What's tested for these is that the user agent come out the right way. This is an integration test. Here we want a unit test: given a specific config, what UA is generated.

sickpig edited edge metadata.

remove net.h from rpc/abc.cpp
remove the float hack to get the the EB value in MB, in the process remove also the inclusion of cfenv
getSubVersionEB still public because we are going to use it also for the coinbase txn and because dunno how to declare as private a function that does not belong to a class

deadalnix requested changes to this revision.Jun 19 2017, 20:33
deadalnix added inline comments.
src/net.cpp
2810 ↗(On Diff #550)

Don't bother about this. I was just wondering if net.cpp is the right place, but it will do.

2776

This is still using floating point. We need integer math because it is deterministic.

src/rpc/abc.cpp
5

I assume this guy is not needed either.

This revision now requires changes to proceed.Jun 19 2017, 20:33
src/net.cpp
2776

I purposely set a higher precision than what we need. 2 rather then 1. And once converted to string I just remove the leftmost digit. Is this is enough for you?

src/net.h
807 ↗(On Diff #550)

I plan to use it also for the EB value to put in the coinbase string. Other than that I really don't know how to declare a function private outside of a class.

src/rpc/abc.cpp
5

right, will fix

sickpig edited edge metadata.

Remove float arithmetic from getSubVersionEB()
Remove clientversion.h from rpc/abc.c

sickpig added inline comments.
src/net.cpp
2776

*rightmost

deadalnix requested changes to this revision.Jun 20 2017, 08:43

Ok this is almost alright. One final touch and this gets in.

src/net.cpp
2778 ↗(On Diff #581)

Maybe keeping the .0 would help automation. Nobody likes special cases.

src/net_processing.cpp
254 ↗(On Diff #581)

This is only called from ProcessMessage, which has the config already, and InitializeNode . Just use GetConfig in InitializeNode and pass it down from other places. Also add a FIXME in InitializeNode that the config should be passed down rather than retrieved that way.

src/qt/clientmodel.cpp
185 ↗(On Diff #581)

For the wallet we don't care.

This revision now requires changes to proceed.Jun 20 2017, 08:43
src/net.cpp
2778 ↗(On Diff #581)

I did the same way BU and Classic did, just for the sake of compatibility.

src/net_processing.cpp
254 ↗(On Diff #581)

Ok will do

sickpig added inline comments.
src/net.cpp
2778 ↗(On Diff #581)

and for what is worth I'm pretty sure BU permit also EB to be lower than 1MB, dunno about classic

src/test/net_tests.cpp
171 ↗(On Diff #581)

I think there should be a test of formatting for blocksize == 0 and for blocksize ~ 0.1MB or so, to make sure that the above operations with .at(...) and so forth on the string also work for these corner cases.

src/test/net_tests.cpp
171 ↗(On Diff #581)

In ABC EB can't be less than 1,000,000, see rpc/abc.cpp

src/test/net_tests.cpp
171 ↗(On Diff #581)

Fair point. I was simply worried that at some point in the future, the above formatting function is called with bogus values from somewhere and might fail hard (out-of-bounds string access exception or such).

src/test/net_tests.cpp
171 ↗(On Diff #581)

I suppose we could add a check on the passed EB value, but if a EB lower than 1MB percolate down here means we failed bad at higher levels. let see what @deadalnix have to say about it.

sickpig edited edge metadata.

Add const Config &config to PushNodeVersion
Use GetConfig() in InitializeNode and pass it down from there.
Did not add to .0 to integer defined EB to keep compatibility with BU and Classic

src/net_processing.cpp
287 ↗(On Diff #583)

passed as arguent

254 ↗(On Diff #581)

Config as first argument.

src/test/net_tests.cpp
171 ↗(On Diff #581)

Small and big as well actually.

deadalnix requested changes to this revision.Jun 20 2017, 11:38
deadalnix added inline comments.
src/net_processing.cpp
254 ↗(On Diff #583)

Config as first argument.

287 ↗(On Diff #583)

dito

src/test/net_tests.cpp
171 ↗(On Diff #583)

This doesn't contained the check we talked about.

This revision now requires changes to proceed.Jun 20 2017, 11:38
sickpig edited edge metadata.

getSubVersionEB works with EB<1MB
getSubVersionEB will rounded down EB in MB keeping a precision of one decimal digit
Document that getSubVersionEB behavior for EB < 1MB is still not standardized
Add tests for EB < 1MB to test/net_tests.cpp
Add tests for EB >> 1MB to test/net_tests.cpp
Adapt test/net_tests.cpp to the new semantic (second item of this list)

deadalnix added inline comments.
src/test/net_tests.cpp
175 ↗(On Diff #585)

Add a check for 0.

This revision is now accepted and ready to land.Jun 20 2017, 12:54

Add a test for EB = 0 to test/net_tests.cpp

sickpig marked 31 inline comments as done.Jun 20 2017, 13:03
sickpig added inline comments.
src/test/net_tests.cpp
175 ↗(On Diff #585)

will do

This revision was automatically updated to reflect the committed changes.
sickpig marked an inline comment as done.