ar_data_sync: Skip unpacking if the chunk was downloaded from the local peer #637

shizzard · 2024-10-27T18:32:50Z

No description provided.

apps/arweave/src/ar_data_sync.erl

…hunk/3`

apps/arweave/src/ar_data_sync.erl

shizzard · 2024-10-29T20:07:49Z

@JamesPiechota I did implement the requested packing of the served chunks for local peers only.

I did test all the possible combinations of packer and miner configurations, including:

for packer:
- setting local_peer address and not setting it (to check the local_peers filtering for the packer);
- setting the pack_served_chunks and not setting it (to check on-demand packing);
for miner:
- setting sync_from_local_peers_only to ensure syncing only from local packer;
- setting data_sync_request_packed_chunks: to test on-demand packing);
- setting local_peer address and not setting it (to check the local_peers filtering for requesting packed chunks);

I didn't find any node misbehaving.

JamesPiechota · 2024-10-30T13:32:09Z

I'm rerunning the auto tests. ar_packing_tests failed - in the past that hasn't been that flaky, so it may indicate a real issue, but re-running just in case.

JamesPiechota · 2024-10-30T13:33:36Z

@JamesPiechota I did implement the requested packing of the served chunks for local peers only.

I did test all the possible combinations of packer and miner configurations, including:

for packer:

setting local_peer address and not setting it (to check the local_peers filtering for the packer);

setting the pack_served_chunks and not setting it (to check on-demand packing);

for miner:

setting sync_from_local_peers_only to ensure syncing only from local packer;

setting data_sync_request_packed_chunks: to test on-demand packing);

setting local_peer address and not setting it (to check the local_peers filtering for requesting packed chunks);

I didn't find any node misbehaving.

Those tests look good! Were you able to confirm when chunk repacking actually occurs on both packer and miner? I think last time we only checked that the requests were formatted correctly, and missed the fact that even with correctly formatted requests, the miner had to do some unpacking to validate.

shizzard · 2024-10-30T13:35:15Z

Were you able to confirm when chunk repacking actually occurs on both packer and miner? I think last time we only checked that the requests were formatted correctly, and missed the fact that even with correctly formatted requests, the miner had to do some unpacking to validate.

No, how can I actually check that the chunk was packed correctly?

JamesPiechota · 2024-10-30T13:40:02Z

apps/arweave/src/ar_http_iface_middleware.erl

@@ -2056,6 +2062,9 @@ handle_get_chunk(OffsetBinary, Req, Encoding) ->
 			{400, #{}, jiffy:encode(#{ error => invalid_offset }), Req}
 	end.

+is_matching_addr({{O1, O2, O3, O4}, _Port1}, {O1, O2, O3, O4, _Port2}) -> true;
+is_matching_addr(_CowboyPeer, _LocalPeer) -> false.


We don't really have a good place for peer-related utilities. But there are a few in ar_util. I think is_matching_addr and maybe even the local_peers fold could live there with the other peer utilities (e.g. maybe ar_util:peer_addr_is_member or something)

JamesPiechota · 2024-10-30T13:48:11Z

Were you able to confirm when chunk repacking actually occurs on both packer and miner? I think last time we only checked that the requests were formatted correctly, and missed the fact that even with correctly formatted requests, the miner had to do some unpacking to validate.

No, how can I actually check that the chunk was packed correctly?

Easiest way is probably to monitor the metrics. Every time a node does a pack, unpack, or repack, the packing_requests metric is incremented. You can use this Grafana board to monitor packing behavior, or also manually check the /metrics endpoint and look at the packing_requests value. If you want to use the grafana board we'll have to add your IP:PORT to our prometheus server, If you DM me those values I can make the addition.

ldmberman · 2024-11-08T10:22:15Z

apps/arweave/src/ar_http_iface_middleware.erl

+								IsLocalPeerAddr = lists:foldl(fun
+									(_, true) -> true;
+									(LocalPeer, _) -> is_matching_addr(Peer, LocalPeer)
+								end, false, Config#config.local_peers),


local_peers should be all in the same format so I think we can just use lists:member

Actually not, that's the problem here.
Cowboy uses standard Erlang address tuple: {{O1, O2, O3, O4}, Port}, while local peers are parsed into five-tuple: {O1, O2, O3, O4, Port}.
Probably should change this, but I'm afraid I can suddenly break something. Should I try?

@shizzard unfortunately it looks ar_util:safe_parse_peer and ar_util:parse_peer are used to parse all config peers and they always dump to the 5 element tuple. I agree probably too risky to update that now, but can you log a github issue? Seems like something we should rationalize with cowboy.

FWIW it looks like the parse_peer code is from 7 years ago (!)

apps/arweave/src/ar_serialize.erl

apps/arweave/src/ar_http_iface_middleware.erl

apps/arweave/src/ar_data_sync.erl

JamesPiechota · 2024-11-10T18:37:28Z

apps/arweave/src/ar_http_iface_middleware.erl

+										ok = ar_semaphore:acquire(get_and_pack_chunk, infinity),
+										{RequestedPacking, ok};
+									{_, _} ->
+										{none, {reply, {403, #{}, <<>>, Req}}}


I think this should be a 404 to match the old behavior: https://github.com/ArweaveTeam/arweave/pull/637/files#diff-0b7084531612365a22d52032a422eb664adc91b34cefb5f8d9499584072aa489L2013

We could have something like {true, false} be a 403

But I think best to just always 404 if the server can't or is unwilling to serve the chunk in the requested format

Ah yes, I did this change to distinct this particular response from others during the tests. Will revert.

Prior to this change ar_tx_blacklist_tests relied on every test node having pack_served_chunks set and being able to repack chunks on demand. To preserve that behavior we have to add each node to the local_peers config for the test nodes. Note: I haven't run this change on other tests yet, it's possible it causes something else to fail (in which case we may want to clear local_peers in that failing test)

Co-authored-by: Lev Berman <[email protected]>

…list

Initialize poa caches for initial block cache blocks

This commit adds the packing difficulty in `v2_index_data_size_by_packing` metric. see: ArweaveTeam/arweave-dev#568

…eer-verification

Skip unpacking if the chunk was downloaded from the local peer

3fe8b47

shizzard requested a review from JamesPiechota October 27, 2024 18:32

JamesPiechota reviewed Oct 28, 2024

View reviewed changes

apps/arweave/src/ar_data_sync.erl Show resolved Hide resolved

shizzard added 2 commits October 28, 2024 16:49

Check for packing correctness when skipping the chunk verification

9b8a178

Reimplement verification skip by reusing the `process_valid_fetched_c…

9d4f815

…hunk/3`

shizzard commented Oct 28, 2024

View reviewed changes

apps/arweave/src/ar_data_sync.erl Show resolved Hide resolved

shizzard added 2 commits October 29, 2024 17:51

Only serve packed chunks for local peers

2550673

Fix packing served chunks filtering with matching the peer address only

b4781c4

JamesPiechota reviewed Oct 30, 2024

View reviewed changes

shizzard added 3 commits October 30, 2024 19:49

Change the way autotests (CI) are run

2b7ad31

Fixup data sync tests

2a1e4e0

Fix packing-on-demand server-side handling

3c05276

shizzard force-pushed the ar_data_sync-sjup-local-peer-verification branch from 7e809c0 to 3c05276 Compare November 4, 2024 15:40

shizzard added 2 commits November 4, 2024 20:03

Fix the data sync test

db96e93

Tests fixes

46f63d0

ldmberman reviewed Nov 8, 2024

View reviewed changes

JamesPiechota reviewed Nov 10, 2024

View reviewed changes

apps/arweave/src/ar_serialize.erl Outdated Show resolved Hide resolved

JamesPiechota reviewed Nov 10, 2024

View reviewed changes

apps/arweave/src/ar_http_iface_middleware.erl Show resolved Hide resolved

JamesPiechota reviewed Nov 10, 2024

View reviewed changes

apps/arweave/src/ar_data_sync.erl Show resolved Hide resolved

JamesPiechota reviewed Nov 10, 2024

View reviewed changes

JamesPiechota and others added 5 commits November 11, 2024 22:25

Initialize poa caches for initial block cache blocks

84ba1ea

Co-authored-by: Lev Berman <[email protected]>

Change the behavior of pack_served_chunks option and local_peers …

5de91fd

…list

Revert local_peers setting for the ar_test_node

1bc997f

Fix reviewers comments

5f5d75c

vird and others added 4 commits November 25, 2024 16:40

Merge pull request #648 from ArweaveTeam/fix/missing_poa_cache2

0ebfa93

Initialize poa caches for initial block cache blocks

Add Packing Difficulty in v2_index_data_size_by_packing (#649)

8fd0010

This commit adds the packing difficulty in `v2_index_data_size_by_packing` metric. see: ArweaveTeam/arweave-dev#568

Address reviewers comments

d2d363c

Merge branch 'release/performance-2.8' into ar_data_sync-sjup-local-p…

a39fde2

…eer-verification

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ar_data_sync: Skip unpacking if the chunk was downloaded from the local peer #637

ar_data_sync: Skip unpacking if the chunk was downloaded from the local peer #637

shizzard commented Oct 27, 2024

shizzard commented Oct 29, 2024

JamesPiechota commented Oct 30, 2024

JamesPiechota commented Oct 30, 2024

shizzard commented Oct 30, 2024

JamesPiechota Oct 30, 2024

JamesPiechota commented Oct 30, 2024

ldmberman Nov 8, 2024

shizzard Nov 8, 2024

JamesPiechota Nov 8, 2024

JamesPiechota Nov 10, 2024

shizzard Nov 10, 2024

ar_data_sync: Skip unpacking if the chunk was downloaded from the local peer #637

Are you sure you want to change the base?

ar_data_sync: Skip unpacking if the chunk was downloaded from the local peer #637

Conversation

shizzard commented Oct 27, 2024

shizzard commented Oct 29, 2024

JamesPiechota commented Oct 30, 2024

JamesPiechota commented Oct 30, 2024

shizzard commented Oct 30, 2024

JamesPiechota Oct 30, 2024

Choose a reason for hiding this comment

JamesPiechota commented Oct 30, 2024

ldmberman Nov 8, 2024

Choose a reason for hiding this comment

shizzard Nov 8, 2024

Choose a reason for hiding this comment

JamesPiechota Nov 8, 2024

Choose a reason for hiding this comment

JamesPiechota Nov 10, 2024

Choose a reason for hiding this comment

shizzard Nov 10, 2024

Choose a reason for hiding this comment