[5.0] Fix base64 encoding - take 2

[heifner]

#1482 fixed base64 encoding by removing the unneeded extra = that variant added to base64 encoded data. However, it also updated the base64 library with a stricter one. This PR leaves the old base64 encoding/decoding library but does remove the extra = from the base64 encoded data so non-fc base64 decoders will not claim the data is invalid.

This change still breaks backward compatibility with 3.2 & 4.0 cleos/nodeos. PRs to 3.2 (#1889) and (#1892) 4.0 will be created that will allow them to work with this new base64 encoding (they will no longer expect the invalid = character).

PR #1886 reverts #1482. This PR provided an alternative fix for #1461.

Included in this PR is an optimization to the existing base64_decode to avoid a copy for our use-cases by returning a vector<char> instead of a string.
Included in this PR is a change to the nodeos_run_test.py to keep the keosd running with passed --leaving-running which is useful for local testing. This was used to manually test using 3.2 cleos with 5.0.

Resolves #1461

[spoonincode]

Included in this PR is an optimization to the existing base64_decode to avoid a copy for our use-cases by returning a vector<char> instead of a string

This has the potential to affect downstream usages when base64_decode("").data() since the return value will be different due to this modification. I didn't spot any problematic usages but it's worth taking a moment to think on it.

[heifner]

This has the potential to affect downstream usages when base64_decode("").data() since the return value will be different due to this modification. I didn't spot any problematic usages but it's worth taking a moment to think on it.

Good point. I'm not a fan of std::string.data() required to be null terminated. In my mind that is what c_str() is for. Not sure why they made that change. I don't buy the removal of COW possibility a valid argument to make data() and c_str() the same thing. Oh well. I didn't find any uses of base64_decode(.*).data() or returns to auto.

[greg7mdp]

I'm not a fan of std::string.data() required to be null terminated. In my mind that is what c_str() is for

The issue is that if std::string.data() was not null terminated, c_str() would have to allocate a new string to add the \0 at the end, and then when would that string be freed?

[greg7mdp]

This has the potential to affect downstream usages when base64_decode("").data() since the return value will be different due to this modification. I didn't spot any problematic usages but it's worth taking a moment to think on it.

For better compatibility, maybe we should add a '\0' at the end of the vector (not included in its size()). This would be lost if the vector is copied, but would address the use case you mentioned above.

[heifner]

For better compatibility, maybe we should add a '\0' at the end of the vector (not included in its size()). This would be lost if the vector is copied, but would address the use case you mentioned above.

You mean append a \0 and then resize by size()-1. Not a fan of that.

[greg7mdp]

Very minor detail

Suggested change

	return blob( { std::move(b64) } );
	return { std::move(b64) };