contrib: change default value of epee::serialization to fix limitation

[0xFFFC0000]

Based on different benchmarks and measurements we realized that 65536 would be the idea number for this limitation.

Co-authored-by: Boog900 <[email protected]>

[0xFFFC0000]

Here are a few benchmarks we did to measure the times to find these limits.

Keep in mind there is 100mb network packet size limit. So we don't need to support anything that goes beyond that.

[0xFFFC0000]

And code for the benchmark is available here [1]. Incase anyone wants to take a look at it.

1. Adding benchmark for (de)serialization 0xFFFC0000/monero#30

[selsta]

What is the RAM usage? We added these limits to avoid DoS packets using too much RAM.

[0xFFFC0000]

@selsta RAM usage was negligible. On my 64gb PC I didn’t notice usage more than 5% when running these benchmarks. That’s why I didn’t measure RAM usage. But I will report the numbers.

[Boog900]

Just to add some more details on how these limits were chosen, the string limit was chosen to allow us to roughly reach the 100mb packet size limit with average size txs.

Then the object limit was chosen to allow us to store the same amount of pruned txs in an GetObjects response as normal txs, the fields limit was chosen the same way.

However looking at the pruned GetObjects response the pruned tx's objects fields are both strings, which means to truly reach the same limit we would need to increase the string limit to 130k.

monero/src/cryptonote_protocol/cryptonote_protocol_defs.h

Lines 121 to 128 in 0db9e74

	struct tx_blob_entry
	{
	blobdata blob;
	crypto::hash prunable_hash;
	BEGIN_KV_SERIALIZE_MAP()
	KV_SERIALIZE(blob)
	KV_SERIALIZE_VAL_POD_AS_BLOB(prunable_hash)
	END_KV_SERIALIZE_MAP()

A 130k string limit may be too high, in which case it might be best to accept that when syncing pruned blocks we are not able to pack as many txs as when syncing full blocks. If we were to accept this we could decrease the object limit to 32k and field limit to 64k.

[iamamyth]

The reported time units in the benchmark don't make sense: For example, in the final tiny_4_string_test, the benchmark reports 13461399ms for "Time" (which I assume means wall clock time) and 3396ms for CPU. Does pausing and resuming the timer break wall clock timing, or are the units here wrong, or something else?

[0xFFFC0000]

The reported time units in the benchmark don't make sense: For example, in the final tiny_4_string_test, the benchmark reports 13461399ms for "Time" (which I assume means wall clock time) and 3396ms for CPU. Does pausing and resuming the timer break wall clock timing, or are the units here wrong, or something else?

I am pausing and resuming the timer when I am creating the objects. Only measuring the epee::(de)serialization operation, if that's what you mean.

You can build the benchmark_serialization test from here [1].

1. Adding benchmark for (de)serialization 0xFFFC0000/monero#30

[iamamyth]

Setting aside my prior comment, assuming the CPU time values are accurate, the required time in many of these scenarios seems too high: In the faster of the two runs (the experimental setup lacks details, such as thread count, affinity, etc, to determine why there's such a huge gap between them), the maximum time is ~23s. The test doesn't split serialization and deserialization, but if one assumes an even split for each operation, that's ~11s spent just to deserialize the message, i.e. ~10% of the expected time between blocks. Aside from implying a lot of net splits, if an attacker can jam up a CPU for 11s at will, that's a big problem.

[iamamyth]

I am pausing and resuming the timer when I am creating the objects. Only measuring the epee::(de)serialization operation, if that's what you mean.

What I mean is the numbers plainly make no sense: Wall time (13461399ms) shouldn't exceed CPU time (3396ms) by a factor of ~4000 for a CPU-bound operation.

[0xFFFC0000]

I am pausing and resuming the timer when I am creating the objects. Only measuring the epee::(de)serialization operation, if that's what you mean.

What I mean is the numbers plainly make no sense: Wall time (13461399ms) shouldn't exceed CPU time (3396ms) by a factor of ~4000 for a CPU-bound operation.

A few points about this:

1. That was an unrelated bug because I don't use for loop operation of the Google Benchmark library. and Pausing and Resuming the timer was not working without GoogleBenchmark for loop.
2. I just wanted CPU runtime.
3. Just to make sure there is no dispute here, I updated the benchmark, ran it and you can find new screenshots here. For most cases they are even better, and as you can see sometimes numbers are roughly the same ( CPU is important for us ). So there is no problem increasing the default_levin_limits.
4. You can compile and run it on your machine. Configure the same as the default monero project with CMake. Instead of building monerod, just run ninja benchmark_serialization or make benchmark_serialization if you are using make. ( Adding benchmark for (de)serialization 0xFFFC0000/monero#30 )
5. It is quite easy to have separate numbers for serialization and (de)serialization. But we don't care about each operation's performance here. We only care that change does not introduce any opportunities for any DDos attack.

-Ofast default compilation:

Hope this helps.

[0xFFFC0000]

Added memory usage to benchmarks too, in case there is any concern about memory usage:

[iamamyth]

The latest numbers look much more reasonable, thanks. I agree with the overall methodology. The high watermark (~17.8 s) is still a bit concerning, but not bad enough that I consider it within the scope of this PR (ultimately, it should be lowered a lot by other work to improve serialization).

[rbrunner7]

@selsta asked a while back here:

What is the RAM usage? We added these limits to avoid DoS packets using too much RAM.

Not sure whether this has already be mentioned and explained clearly enough, but the limits that this PR wants to rise were introduced as a reaction to an attack on the Monero network. As far as I understand daemons were crashed using specially crafted packets that, while themselves reasonably small, blew up to an enormous size in RAM when deserialized. You can read a bit about the attack here on Reddit.

The PR that introduced these limits at the start of 2021 is [#7286].

It seems that knowledge exactly how that attack worked is lost, unfortunately. Which, like @selsta probably as well, I find mildly worrying: Under these circumstances, how can we be reasonably sure we do not re-enable such an attack by larger limits?

We are probably safe, I would say. But on the other hand I don't think like an attacker ...

[vtnerd]

It seems that knowledge exactly how that attack worked is lost, unfortunately

The details are not lost. #7999 and #8867 both contain tests that simulate the attack.

The primary mechanism for the attack was serializing 0-length strings or 0-length objects inside of an array. Each element was ~1 byte on the wire compared to ~128 bytes in the DOM. The ratio of 1/128 meant that you could have unpacked size ~12.5 GiB. IIRC, the first attempt to stop it failed because it enforced limits per array instead of global.

[vtnerd]

It seems that knowledge exactly how that attack worked is lost, unfortunately

The details are not lost. #7999 and #8867 both contain tests that simulate the attack.

The primary mechanism for the attack was serializing 0-length strings or 0-length objects inside of an array. Each element was ~1 byte on the wire compared to ~128 bytes in the DOM. The ratio of 1/128 meant that you could have unpacked size ~12.5 GiB. IIRC, the first attempt to stop it failed because it enforced limits per array instead of global.

I also forgot fields. The unpacked size is at least 160 bytes (not including std::map overhead per entry), and the on-wire length varies due to string length. But the ratio is definitely pretty large there too.

[rbrunner7]

Thanks for the interesting details, @vtnerd ! They make sense, and basically make my worries go away.