Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Spec'ing creative scanning via BYOS/V1 trusted scoring signals #1398

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

morlovich
Copy link
Collaborator

@morlovich morlovich commented Feb 10, 2025

This also fixes trusted scoring signals params escaping in general to not escape commas
when not expected.

See #792 (comment)
and latter comments on that issue for more context.


Preview | Diff

@JensenPaul JensenPaul added the spec Relates to the spec label Feb 11, 2025
@morlovich morlovich changed the title WIP: working on spec'ing creative scanning Spec'ing creative scanning via BYOS/V1 trusted scoring signals Feb 13, 2025
@morlovich morlovich requested a review from orrb1 February 13, 2025 16:07
Copy link
Collaborator

@orrb1 orrb1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, Maks!

Copy link
Collaborator

@orrb1 orrb1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, Maks!

@morlovich morlovich requested a review from qingxinwu February 18, 2025 14:17
@morlovich
Copy link
Collaborator Author

I suspect the failure is due to speced/bikeshed-boilerplate@123d968

1. Let |sendCreativeScanningMetadata| be |auctionConfig|'s [=auction config/send creative scanning
metadata=].
1. If |auctionConfig|'s [=auction config/trusted scoring signals coordinator=] is not null,
set |sendCreativeScanningMetadata| to false.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mind pointing me to the code?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so is "hasPublicKey()" the same as coordinator not being null here? When coordinator is not null, the key fetch can also fail (spec) which causes no public key, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So in the spec at the moment, if the key fetch fails, it just fails the V2 fetch path.
The impl currently falls back (which that code incorporates since the key fetch happens before we get to it), but I think that fallback may no longer happen when the cache is on, which would then match the spec, both my addition and the actual fetch stuff?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, yeah I heard about the cache stuff would change things, but I'm not familiar with it.

spec.bs Outdated
@@ -8357,6 +8403,9 @@ An <dfn>interest group ad</dfn> is a [=struct=] with the following [=struct/item
: <dfn>ad render ID</dfn>
:: A [=string=] containing up to 12 [=ASCII bytes=] uniquely identifying this ad. Sent instead
of the full [=interest group ad=] for auctions executed on a server.
: <dfn>creative scanning metadata</dfn>
:: Null or a [=string=], initially null. Sent with V1 trusted scoring signals requests if
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the spec has a definition of our concept of "V1". Can we have a (one or multiple sentences) definition of it in the spec, or replae V1 with something more explicit?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe non-TEE?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's good as long as that's clear to readers. Otherwise, a definition with short explaination can be helpful.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rephrased

1. If |experimentGroupId| is not null:
1. [=list/Append=] "`&experimentGroupId=`" to |queryParamsList|.
1. [=list/Append=] [=serialize an integer|serialized=] |experimentGroupId| to |queryParamsList|.
1. If |sendCreativeScanningMetadata| is true:
1. [=Add trusted signals param if needed=] given "`&adCreativeScanningMetadata=`",
the result of [=extracting creative scanning metadata=] given |ads|, |queryParamsList|.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: it's kinda confusing about which param is for which algorithm, but I don't see a better way to reword it.

Copy link
Collaborator Author

@morlovich morlovich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Applied the suggestions..

spec.bs Outdated
@@ -8357,6 +8403,9 @@ An <dfn>interest group ad</dfn> is a [=struct=] with the following [=struct/item
: <dfn>ad render ID</dfn>
:: A [=string=] containing up to 12 [=ASCII bytes=] uniquely identifying this ad. Sent instead
of the full [=interest group ad=] for auctions executed on a server.
: <dfn>creative scanning metadata</dfn>
:: Null or a [=string=], initially null. Sent with V1 trusted scoring signals requests if
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rephrased

Copy link
Collaborator

@qingxinwu qingxinwu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

last few comments.

A <dfn>trusted scoring signals request</dfn> is a [=struct=] with the following [=struct/items=],
representing all information needed to produce a trusted scoring signals fetch for a single scoring
invocation:

<dl dfn-for="trusted scoring signals request">
: <dfn>send creative scanning metadata</dfn>
:: A [=boolean=], to determine whether creative scanning metadata should be added to V1 requests.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one more v1.

[=creative info=] |componentAds|, an {{unsigned short}} |experimentGroupId|, and an [=origin=]
|topLevelOrigin|:

Note: When trusted scoring signals fetches are not batched, |ads|'s [=list/size=] is 1.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Note: When trusted scoring signals fetches are not batched, |ads|'s [=list/size=] is 1.
Note: When trusted scoring signals fetches are not batched, |ads|'s [=set/size=] is 1.


1. Let |queryParamsList| be a new empty [=list=].

Copy link
Collaborator

@qingxinwu qingxinwu Feb 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: extra white line, due to my previous code suggestion.

Suggested change

|queryParamsList|.
1. Let |adComponentRenderURLs| be the result of [=extracting URLs from creative info=] given
|componentAds|.
1. If |adComponentRenderURLs| is not [=list/is empty|empty=]:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[=list/is not empty=] should also work.

Suggested change
1. If |adComponentRenderURLs| is not [=list/is empty|empty=]:
1. If |adComponentRenderURLs| [=list/is not empty=]:

To <dfn>extract component ad creative scanning metadata</dfn> given a [=list=] of
[=interest group ad=] |bidAdComponents|:

1. Let |result| be a new <code>[=sequence=]<{{USVString}}></code>.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

each element can be null, right?

Suggested change
1. Let |result| be a new <code>[=sequence=]<{{USVString}}></code>.
1. Let |result| be a new <code>[=sequence=]<{{USVString?}}></code>.

1. Let |mainAd| be the result of [=filling in creative info=] given |sendCreativeScanningMetadata|,
|generatedBid|'s [=generated bid/ad descriptor=], |owner|, [=generated bid/bid ad=].
1. Let |adComponents| be a new empty [=set=].
1. For |i| between 0 and |generatedBid|'s [=generated bid/bid ad components=]'s [=list/size=] - 1,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
1. For |i| between 0 and |generatedBid|'s [=generated bid/bid ad components=]'s [=list/size=] - 1,
1. [=list/for each=] |i| in [=the range=] from 0 to |generatedBid|'s [=generated bid/bid ad components=]'s [=list/size=] - 1,

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
spec Relates to the spec
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants