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

Explicitly spec out how non-k-anon auction works. #1080

Merged
merged 10 commits into from
Mar 18, 2024

Conversation

morlovich
Copy link
Collaborator

@morlovich morlovich commented Mar 8, 2024

... This is a prerequisite to specing how it works with targetNumAdComponents, which has rather subtler
effects on what bids are made in each auction.


Preview | Diff

... This is a prereq to specing how it works with
targetNumAdComponents.
@morlovich morlovich added the spec Relates to the spec label Mar 8, 2024
@morlovich morlovich requested a review from qingxinwu March 8, 2024 19:00
1. [=Score and rank a bid=] with |auctionConfig|, |generatedBid|, |leadingBidInfo|,
|decisionLogicScript|, |directFromSellerSignalsForSeller|, |dataVersion|, |auctionLevel|,
|componentAuctionExpectedCurrency|, and |topLevelOrigin|.
1. If |generatedBid| is not a failure:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm, might need to register the original generatedBid for debugging if this one is a failure to get the reject reason = below threshold stuff out?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Though this appears that it may only work for PA, not for forDebuggingOnly?

https://source.chromium.org/chromium/chromium/src/+/main:content/browser/interest_group/interest_group_auction.cc;drc=3afa53375890e7144673d8e9e6356868295c2873;l=490

... which may make my pull request to the explainer a bit off :(

Copy link
Collaborator

Choose a reason for hiding this comment

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

hmm, seems like a bug, that we may want to report this k-anon reject reason to forDebuggingOnly as well?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, probably. And I think my next refactors of this make us better suited for speccing it, so maybe don't need to worry about this for a moment.

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.

sorry for the delay. Some quick comments, and still need to take another look.

spec.bs Outdated
@@ -5088,6 +5128,9 @@ result of [=evaluating a bidding script=], or an [=additional bid=] provided by
: <dfn>no bid</dfn>
:: A [=boolean=], initially false. True if there was a failure generating a bid, or the bidder
chose not to bid. When true, [=generated bid/bidder debug loss report url=] is still considered.
: <dfn>for k-anon auction</dfn>
:: A [=boolean=], initially true. If this is false, the bid is only used to determine the hypothetical
winner with no k-anonymity constraints, which would be used to update k-anonymity counts only.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we link "update k-anonymity counts" to an algorithm, maybe [=Increment a winning bid's k-anonymity count=]?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What's the syntax for linking to an algorithm with text that's not just its name?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks Dominic; done. (Though maybe I will want to tweak the name further)

spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated
1. [=iteration/continue=].
1. Let |bidsToScore| be a new [=list=] of [=generated bids=]
1. If [=query generated bid k-anonymity count=] given |generatedBid| returns true:
1. Let |bidCopy| be a copy of |generatedBid|.
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe "copy" -> "clone".
@domfarolino Is there a [=clone=] to link to for struct? I saw lists/sets/maps have one, but didn't find one for struct .

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah infra doesn't seem to have a definition for this and it probably should. Could you use the word "clone" manually, and then file a bug against the Infra Standard?

Copy link
Collaborator

Choose a reason for hiding this comment

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

spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
@morlovich morlovich mentioned this pull request Mar 14, 2024
1. [=Score and rank a bid=] with |auctionConfig|, |generatedBid|, |leadingBidInfo|,
|decisionLogicScript|, |directFromSellerSignalsForSeller|, |dataVersion|, |auctionLevel|,
|componentAuctionExpectedCurrency|, and |topLevelOrigin|.
1. If |generatedBid| is not a failure:
Copy link
Collaborator

Choose a reason for hiding this comment

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

hmm, seems like a bug, that we may want to report this k-anon reject reason to forDebuggingOnly as well?

spec.bs Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
@dmdabbs
Copy link
Contributor

dmdabbs commented Mar 18, 2024

correctly I can order word.

😂

Copy link
Collaborator

@domfarolino domfarolino left a comment

Choose a reason for hiding this comment

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

This mostly looks fine to me, I think I will defer to @qingxinwu on this PR.

spec.bs Outdated
@@ -1700,7 +1700,7 @@ To <dfn>generate and score bids</dfn> given an [=auction config=] |auctionConfig
|generatedBidsForDebuggingReports|.
1. [=list/For each=] |bidToScore| of |bidsToScore|:
1. If |bidToScore|'s [=generated bid/for k-anon auction=] is true,
[=list/insert=] |bidToScore|'s [=generated bid/interest group=] in |bidIgs|.
[=list/append=] |bidToScore|'s [=generated bid/interest group=] in |bidIgs|.
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: append ... to ...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done, thanks

spec.bs Outdated
1. [=iteration/continue=].
1. Let |bidsToScore| be a new [=list=] of [=generated bids=]
1. If [=query generated bid k-anonymity count=] given |generatedBid| returns true:
1. Let |bidCopy| be a copy of |generatedBid|.
Copy link
Collaborator

Choose a reason for hiding this comment

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

@qingxinwu
Copy link
Collaborator

This mostly looks fine to me, I think I will defer to @qingxinwu on this PR.

Just wanted to add you to review this, but you beat me. It LGTM % a nit.

@JensenPaul JensenPaul merged commit 28abf64 into WICG:main Mar 18, 2024
2 checks passed
github-actions bot added a commit that referenced this pull request Mar 18, 2024
SHA: 28abf64
Reason: push, by JensenPaul

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
github-actions bot added a commit to morlovich/turtledove that referenced this pull request Mar 18, 2024
SHA: 28abf64
Reason: push, by morlovich

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
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.

5 participants