Add Oracle limit by shargon · Pull Request #800 · neo-project/neo-modules

shargon · 2023-04-27T17:22:52Z

No description provided.

shargon · 2023-04-27T17:24:52Z

@dotnet-policy-service agree

@shargon please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
@dotnet-policy-service agree [company="{your company}"]
Options:

(default - no company specified) I have sole ownership of intellectual property rights to my Submissions and I am not making Submissions in the course of work for my employer.
@dotnet-policy-service agree
(when company given) I am making Submissions in the course of work for my employer (or my employer has intellectual property rights in my Submissions by contract or applicable law). I have permission from my employer to make Submissions and enter into this Agreement on behalf of my employer. By signing below, the defined term “You” includes me and my employer.
@dotnet-policy-service agree company="Microsoft"
Contributor License Agreement

shargon · 2023-04-27T17:25:25Z

@roman-khimov Please, could you take a look?

src/OracleService/Protocols/OracleHttpsProtocol.cs

erikzhang · 2023-04-29T05:11:31Z

Is the response size of NeoFS also limited?

AnnaShaleva · 2023-05-02T10:36:19Z

Is the response size of NeoFS also limited?

It should be limited by the client, but currently it's not. So the problem exists for OracleNeoFSProtocol as far. The size of a single physical NeoFS object is limited by 64MB in mainnet\testnet, but there are large ("virtual") objects constructed from a set of small physical ones and these are almost unlimited.

The neofs-api-csharp Client is capable to handle such large "virtual" objects and doesn't restrict their maximum size:
https://github.com/neo-ngd/neofs-api-csharp/blob/82ca3de1d87c96a04bdf094c60c7c6039bbacf81/src/Neo.FileStorage.API/client/Client.Object.cs#L106-L123
OracleNeoFSProtocol uses both client.GetObject and client.GetObjectPayloadRangeData, reads object data from the stream and converts the potentially large result directly to string:

neo-modules/src/OracleService/Protocols/OracleNeoFSProtocol.cs

Lines 97 to 101 in 4e31068

    
           private static async Task<string> GetPayloadAsync(Client client, Address addr, CancellationToken cancellation) 
        
           { 
        
               Object obj = await client.GetObject(addr, options: new CallOptions { Ttl = 2 }, context: cancellation); 
        
               return obj.Payload.ToString(Utility.StrictUTF8); 
        
           }

and

neo-modules/src/OracleService/Protocols/OracleNeoFSProtocol.cs

Lines 103 to 108 in 4e31068

    
           private static async Task<string> GetRangeAsync(Client client, Address addr, string[] ps, CancellationToken cancellation) 
        
           { 
        
               if (ps.Length == 0) throw new FormatException("missing object range (expected 'Offset|Length')"); 
        
               Range range = ParseRange(ps[0]); 
        
               var res = await client.GetObjectPayloadRangeData(addr, range, options: new CallOptions { Ttl = 2 }, context: cancellation); 
        
               return Utility.StrictUTF8.GetString(res);

Preferred solution

Ideally, to solve this problem we need a change in the neofs-api-csharp Client's API to limit the amount of data that can be read from the stream. In neofs-sdk-go it is done by returning the ObjectReader structure so that it's the reader responsibility to restrict the amount of data that can be read from the underlying transmission channel: https://pkg.go.dev/github.com/nspcc-dev/neofs-sdk-go@v1.0.0-rc.8/client#Client.ObjectGetInit.

Another possible solution

Another way to solve this problem without neofs-api-csharp Client's API changes is to use GetObjectPayloadRangeData combined with manually pre-checked range lenght on the OracleNeoFSProtocol side for both GetObjectRange and GetObject operations. However, this way requires to access and check object headers firstly and is a "kludgy" way in general.

Co-authored-by: Erik Zhang <erik@neo.org>

AnnaShaleva

We should also consider fixing OracleNeoFSProtocol wrt #800 (comment).

AnnaShaleva · 2023-05-03T07:27:17Z

src/OracleService/Protocols/OracleHttpsProtocol.cs

-        private readonly HttpClient client = new(new HttpClientHandler() { AllowAutoRedirect = false });
+        private readonly HttpClient client = new(new HttpClientHandler() { AllowAutoRedirect = false })
+        {
+            MaxResponseContentBufferSize = OracleResponse.MaxResultSize


We should probably adjust the exception handler of OracleService wrt this change. Currently if the responce content exceeds OracleResponse.MaxResultSize length, the exception will be thrown by message.Content.ReadAsStringAsync(...) method:

neo-modules/src/OracleService/Protocols/OracleHttpsProtocol.cs

Line 85 in 4e31068

return (OracleResponseCode.Success, await message.Content.ReadAsStringAsync(cancellation));

It will be catched by the following catch block:

neo-modules/src/OracleService/OracleService.cs

Lines 361 to 368 in 4e31068

try

{

return await protocol.ProcessAsync(uri, ctsLinked.Token);

}

catch (Exception ex)

{

return (OracleResponseCode.Error, $"Request <{url}> Error:{ex.Message}");

}

As a result, we'll have OracleResponseCode.Error code instead of OracleResponseCode.ResponseTooLarge which is more appropriate in this case. In neo-go we return OracleResponseCode.ResponseTooLarge in this case.

Which exception is throw?

It's up to you to investigate which one. The documentation doesn't name it: https://learn.microsoft.com/en-us/dotnet/api/system.net.http.httpclient.maxresponsecontentbuffersize?view=net-8.0#remarks.

@superboyiii could you test it?

@superboyiii could you test it?

Sure

@superboyiii could you test it?

@shargon Due to my test result, the same as @AnnaShaleva said. We need solve it.

…-modules into add-oracle-limit

ZhangTao1596 · 2023-05-12T08:15:01Z

We should also consider fixing OracleNeoFSProtocol wrt #800 (comment).

@AnnaShaleva @shargon There is a stream version GetObject here https://github.com/neo-ngd/neofs-api-csharp/blob/82ca3de1d87c96a04bdf094c60c7c6039bbacf81/src/Neo.FileStorage.API/client/Client.Object.cs#LL39C9-L39C9
We can limit read payload size in stream.

src/OracleService/Protocols/OracleHttpsProtocol.cs

Co-authored-by: Owen Zhang <38493437+superboyiii@users.noreply.github.com>

roman-khimov · 2023-05-17T07:13:24Z

src/OracleService/Protocols/OracleHttpsProtocol.cs

                return (OracleResponseCode.Error, message.StatusCode.ToString());
            if (!Settings.Default.AllowedContentTypes.Contains(message.Content.Headers.ContentType.MediaType))
                return (OracleResponseCode.ContentTypeNotSupported, null);
+            if (!message.Content.Headers.ContentLength.HasValue || message.Content.Headers.ContentLength > OracleResponse.MaxResultSize)


The !message.Content.Headers.ContentLength.HasValue part of it may affect legitimate responses that fit the limit, but don't have the header for whatever reason (dynamically generated content, for example).

We can require this header

That'd be an artificial limitation to me. It shouldn't be hard to handle !message.Content.Headers.ContentLength.HasValue case as well, NeoGo handles both cases easily.

Extracted from: https://github.com/microsoft/referencesource/blob/51cf7850defa8a17d815b4700b67116e3fa283c2/System/net/System/Net/Http/HttpContent.cs#L91

…-modules into add-oracle-limit

AnnaShaleva · 2023-05-18T11:27:50Z

src/OracleService/Protocols/OracleHttpsProtocol.cs

+            byte[] buffer = new byte[OracleResponse.MaxResultSize];
+            var stream = message.Content.ReadAsStream(cancellation);
+            var read = await stream.ReadAsync(buffer, 0, buffer.Length, cancellation);


If response stream contains more than buffer.Length bytes, then it's an error and we need to return OracleResponseCode.ResponseTooLarge code. And if I'm not mistaken, the current code will just stop reading and return a part of the response with OracleResponseCode.Success code (which isn't the desired behaviour).

Please, check the way how it's done in neo-go: https://github.com/nspcc-dev/neo-go/blob/7b86a54fc0dd918a2514b0108be986a6e2cc63fa/pkg/services/oracle/response.go#L71-L82

Please, check it again

This part LGTM, resolved.

AnnaShaleva

@shargon, could you also consider fixing the same problem for OracleNeoFSProtocol as @ZhangTao1596 suggested? #800 (comment)

src/OracleService/Protocols/OracleHttpsProtocol.cs

…-modules into add-oracle-limit

src/OracleService/Protocols/OracleHttpsProtocol.cs

Co-authored-by: Anna Shaleva <shaleva.ann@gmail.com>

AnnaShaleva

LGTM!

shargon · 2023-06-30T09:29:44Z

@superboyiii could you test it?

superboyiii · 2023-07-10T07:45:54Z

@superboyiii could you test it?

I'm ing...

superboyiii

Request: https://api.github.com/users?since=800&per_page=1000

Works as expected!

Add oracle limit

a94bb71

shargon requested a review from superboyiii April 27, 2023 17:25

roman-khimov reviewed Apr 27, 2023

View reviewed changes

src/OracleService/Protocols/OracleHttpsProtocol.cs Outdated Show resolved Hide resolved

AnnaShaleva reviewed Apr 27, 2023

View reviewed changes

src/OracleService/Protocols/OracleHttpsProtocol.cs Outdated Show resolved Hide resolved

AnnaShaleva review

0636c75

erikzhang reviewed Apr 29, 2023

View reviewed changes

src/OracleService/Protocols/OracleHttpsProtocol.cs Outdated Show resolved Hide resolved

Update src/OracleService/Protocols/OracleHttpsProtocol.cs

2568a94

Co-authored-by: Erik Zhang <erik@neo.org>

shargon requested review from AnnaShaleva, erikzhang and roman-khimov May 2, 2023 11:53

AnnaShaleva reviewed May 3, 2023

View reviewed changes

AnnaShaleva mentioned this pull request May 10, 2023

Neo v3.6.0 Checklist neo-project/neo#2844

Closed

shargon added 2 commits May 10, 2023 10:46

Add Response length

1ca97de

Merge branch 'add-oracle-limit' of https://github.com/neo-project/neo…

fbe5ad4

…-modules into add-oracle-limit

superboyiii suggested changes May 15, 2023

View reviewed changes

src/OracleService/Protocols/OracleHttpsProtocol.cs Outdated Show resolved Hide resolved

Update src/OracleService/Protocols/OracleHttpsProtocol.cs

b6a6556

Co-authored-by: Owen Zhang <38493437+superboyiii@users.noreply.github.com>

superboyiii approved these changes May 17, 2023

View reviewed changes

superboyiii previously approved these changes May 17, 2023

View reviewed changes

roman-khimov reviewed May 17, 2023

View reviewed changes

shargon added 3 commits May 17, 2023 10:32

Merge branch 'master' into add-oracle-limit

5d3a824

New version

0fd9dbd

Extracted from: https://github.com/microsoft/referencesource/blob/51cf7850defa8a17d815b4700b67116e3fa283c2/System/net/System/Net/Http/HttpContent.cs#L91

Merge branch 'add-oracle-limit' of https://github.com/neo-project/neo…

83b66b3

…-modules into add-oracle-limit

shargon dismissed superboyiii’s stale review via 83b66b3 May 18, 2023 10:15

shargon added 2 commits May 18, 2023 12:35

Fix

a782cab

Clean

73839c5

AnnaShaleva reviewed May 18, 2023

View reviewed changes

Read one more

bb5a5db

AnnaShaleva reviewed May 19, 2023

View reviewed changes

src/OracleService/Protocols/OracleHttpsProtocol.cs Show resolved Hide resolved

ZhangTao1596 mentioned this pull request May 29, 2023

Oracle: neofs result size limit #803

Merged

ZhangTao and others added 3 commits June 2, 2023 03:00

oracle: neofs result size limit (#803)

b7839fc

Enforce UTF8

97552cc

Merge branch 'add-oracle-limit' of https://github.com/neo-project/neo…

2cc4e53

…-modules into add-oracle-limit

AnnaShaleva approved these changes Jun 9, 2023

View reviewed changes

AnnaShaleva reviewed Jun 9, 2023

View reviewed changes

src/OracleService/Protocols/OracleHttpsProtocol.cs Outdated Show resolved Hide resolved

Update src/OracleService/Protocols/OracleHttpsProtocol.cs

b6e6006

Co-authored-by: Anna Shaleva <shaleva.ann@gmail.com>

AnnaShaleva approved these changes Jun 18, 2023

View reviewed changes

Jim8y approved these changes Jul 5, 2023

View reviewed changes

shargon requested a review from superboyiii July 5, 2023 17:23

superboyiii approved these changes Jul 11, 2023

View reviewed changes

superboyiii merged commit 99ffc84 into master Jul 12, 2023

superboyiii deleted the add-oracle-limit branch July 12, 2023 03:36

	try
	{
	return await protocol.ProcessAsync(uri, ctsLinked.Token);
	}
	catch (Exception ex)
	{
	return (OracleResponseCode.Error, $"Request <{url}> Error:{ex.Message}");
	}

Conversation

shargon commented Apr 27, 2023

Uh oh!

shargon commented Apr 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shargon commented Apr 27, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

erikzhang commented Apr 29, 2023

Uh oh!

AnnaShaleva commented May 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Preferred solution

Another possible solution

Uh oh!

AnnaShaleva left a comment

Choose a reason for hiding this comment

Uh oh!

AnnaShaleva May 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ZhangTao1596 commented May 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AnnaShaleva left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

AnnaShaleva left a comment

Choose a reason for hiding this comment

Uh oh!

shargon commented Jun 30, 2023

Uh oh!

superboyiii commented Jul 10, 2023

Uh oh!

superboyiii left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

shargon commented Apr 27, 2023 •

edited

Loading

AnnaShaleva commented May 2, 2023 •

edited

Loading

AnnaShaleva May 3, 2023 •

edited

Loading

ZhangTao1596 commented May 12, 2023 •

edited

Loading