contrib: fix serialization not calling load when only child is opt

[0xFFFC0000]

The problem is monero-wallet-rpc does not save the state of the wallet when closing the wallet.

As you can see in the image below, the value of autosave_current is false. When in fact it should be true [1]. ( The command I am running is default command curl http://localhost:18082/json_rpc -d '{"jsonrpc":"2.0","id":"0","method":"close_wallet"}' -H 'Content-Type: application/json' )

The reason for this bug is when the only child of the key-value serialization object is optional ( as is the case with the close_wallet ), we don't call the function _load of the object. This line [2] returns null and therefore we don't call _load method, which initializes the object. Whether the child section is null or not, we want to call _load method, to make sure the correct initialization.

1. monero/src/wallet/wallet_rpc_server_commands_defs.h

   Line 2199 in 893916a

   KV_SERIALIZE_OPT(autosave_current, true)

2. monero/contrib/epee/include/serialization/keyvalue_serialization_overloads.h

   Line 80 in 893916a

   if(!hchild_section) return false;

[vtnerd]

This might be a bad idea. The storage class assumes that nullptr means root in several places. Couldn't this cause some loop to occur? I'd have to dig deeper.

More importantly, shouldn't this return a JSON error though, since there is no "params":{} field? This line that doesn't check the return value seems odd, every field is in optional then. Surprised I never noticed this.

[iamamyth]

Normally JSON-RPC doesn't require params:

params
A Structured value that holds the parameter values to be used during the invocation of the method. This member MAY be omitted.
https://www.jsonrpc.org/specification

[0xFFFC0000]

This might be a bad idea. The storage class assumes that nullptr means root in several places. Couldn't this cause some loop to occur? I'd have to dig deeper.

More importantly, shouldn't this return a JSON error though, since there is no "params":{} field? This line that doesn't check the return value seems odd, every field is in optional then. Surprised I never noticed this.

I have a question. _load is written in a way to handle nullptr, so why we should not run it? I don't see any reason to prevent running the _load when the value is nullptr, since _load itself does handle nullptr.

I am not sure about the (infinite) loop thing. Can you explain it in a little bit more detail?

1. monero/contrib/epee/include/serialization/keyvalue_serialization.h

   Line 53 in 893916a

   bool _load( t_storage& stg, typename t_storage::hsection hparent_section = nullptr)\

[Boog900]

Normally JSON-RPC doesn't require params:

This is the solution I suggested: #9573 it will add the params field for JSON-RPC if it is missing.

The error for a missing field not being returned though is also an issue that should be fixed.

[0xFFFC0000]

I added few testcases to PR.

The important test is:

  // test case with empty json, to test default value initialization
  ParentObjWithOptChild<ObjWithOptChild> o4;
  std::string o4_json = "{}";

  EXPECT_TRUE(epee::serialization::load_t_from_json(o4, o4_json));
  EXPECT_TRUE(o4.params.test_value);

[0xFFFC0000]

I am closing this PR. and re-opening #9574 PR.

[vtnerd]

Even though this is closed, I will answer.

I am not sure about the (infinite) loop thing. Can you explain it in a little bit more detail?

I don't think this is possible after further thought. However, it does "pollute" the root namespace into other contexts, so it's possibly still undesired.

Your other solution is funky, but at least fixes all of the problems without introducing other issues.