test: Use byte unit 'M' for -maxuploadtarget functional test #23648

No description provided.

I agree with the PR’s idea, but IMO this test would be redundant as the condition is being added. Because:

• The arguments provided through maxuploadtarget are passed through the ParseByUnit function in init.cpp. Reference: https://github.com/bitcoin/bitcoin/blob/099325a7594f8187f9067d5bff8390084cceb9dd/src/init.cpp#L1114
• And the exact condition (that this PR is introducing) has already been tested in ParseByUnit’s tests. Reference: https://github.com/bitcoin/bitcoin/blob/099325a7594f8187f9067d5bff8390084cceb9dd/src/util/strencodings.cpp#L543-L545

So, though I agree with the idea, I think this PR is redundant.

So, though I agree with the idea, I think this PR is redundant.

Think of this as a sanity check. It may seem redundant now, but as time goes on and more code is added this may catch some edge case not known today. Lastly, the cost is about 1 second more of testing time.

The arg is used twice in the test, if you pass M once and not the other time, you get the same coverage at twice the speed?

The arg is used twice in the test, if you pass M once and not the other time, you get the same coverage at twice the speed?

My main intent of this PR is to verify ParseByteUnits is call within init.cpp:

https://github.com/bitcoin/bitcoin/blob/099325a7594f8187f9067d5bff8390084cceb9dd/src/init.cpp#L1113-L1117

And that 1 and 1M behave the same (that the default byte unit is M).

If 1M is sufficient, I'm fine using just this.

You can also pick one randomly on the random seed, to converge to the same test coverage at no runtime cost.

I now see what you are saying. I changed it to to simply change 1 to 1M

Changes since my last review:

• Updated PR, so that now instead of checking for both ‘1’ and ‘1M’ the test now checks only for ‘1M’

I tried to wrap my head around this change, but I seemed to can’t understand one thing. How, by doing this change, are we able to check this condition:

that 1 and 1M behave the same (that the default byte unit is M). @dougEfresh. It would be pretty helpful to add the logic behind the recent changes done to this PR in the PR’s description. Thank you.

that 1 and 1M behave the same (that the default byte unit is M).

You are correct, this isn't validating this condition. I think it makes more sense to put the byte unit on the 800 :

https://github.com/bitcoin/bitcoin/blob/529ed3336205e30d94e741a4eb93dd945ad518e7/test/functional/feature_maxuploadtarget.py#L38-L41

As you mentioned early there are enough unit tests to validate parsing and amounts...etc

By using 800M I validate byte units don't crash bitcoind, while leaving 1 would be validating that the default byte unit is M

I think that's good enough, what you think?

By using 800M I validate byte units don't crash bitcoind, while leaving 1 would be validating that the default byte unit is M

That sounds like a better idea. This would also make sure using the M suffix is working correctly, along with other advantages you mentioned. So if I am correct, the ultimate patch would look something like this:

diff: master..PR

self.extra_args = [[
-            "-maxuploadtarget=800",
+            "-maxuploadtarget=800M",
             "-acceptnonstdtxn=1",
         ]]

@shaavan correct.

41b9f7d062a01cf39b6dba02703213ffb1951a37 reflects this change

ACK 41b9f7d062a01cf39b6dba02703213ffb1951a37

ACK 41b9f7d.