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

Fix get_custody_columns #3748

Merged
merged 4 commits into from
May 7, 2024
Merged

Fix get_custody_columns #3748

merged 4 commits into from
May 7, 2024

Conversation

hwwhww
Copy link
Contributor

@hwwhww hwwhww commented May 6, 2024

Replace #3742
Thank @nalepae for raising the issue, and thank @zilm13 for proposing a fix for the bug. 🙏

@hwwhww hwwhww added the EIP-7594 PeerDAS label May 6, 2024
Copy link
Member

@ralexstokes ralexstokes left a comment

Choose a reason for hiding this comment

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

I would maybe just leave this variable as i, otherwise it looks good!

also could consider mixing in a % operator instead, but it would end up having the same effect as this code, and this seems fine enough

specs/_features/eip7594/das-core.md Outdated Show resolved Hide resolved
specs/_features/eip7594/das-core.md Outdated Show resolved Hide resolved
specs/_features/eip7594/das-core.md Outdated Show resolved Hide resolved
specs/_features/eip7594/das-core.md Outdated Show resolved Hide resolved
if node_id == UINT256_MAX:
node_id = NodeID(0)
# Overflow prevention
if tmp_id == UINT256_MAX:
Copy link
Contributor

Choose a reason for hiding this comment

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

It makes more sense to move this part of the code below, just before the increment like @dankrad did in #3752, so we will not skip uint256_max iteration step

@@ -106,18 +106,20 @@ def get_custody_columns(node_id: NodeID, custody_subnet_count: uint64) -> Sequen
assert custody_subnet_count <= DATA_COLUMN_SIDECAR_SUBNET_COUNT

subnet_ids: List[uint64] = []
i = 0
i = uint256(node_id)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah sorry @ralexstokes I think I have the revert the previous commit. There is another i usage in the return sorted(...)... so it's less good to assign an i here.

@hwwhww hwwhww force-pushed the fix-get_custody_columns branch from b256bba to c9e0e6d Compare May 7, 2024 13:47
@hwwhww hwwhww merged commit 313a64e into dev May 7, 2024
26 checks passed
@hwwhww hwwhww deleted the fix-get_custody_columns branch May 7, 2024 14:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
EIP-7594 PeerDAS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants