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

get_custody_columns: Move node_id test against UINT256_MAX out of the while loop. #3742

Closed
wants to merge 2 commits into from

Conversation

nalepae
Copy link
Contributor

@nalepae nalepae commented May 2, 2024

node_id is not modified in the loop, and moving the check against UINT256_MAX does not impact specification readability.

…of the `while` loop.

`node_id` is not modified in the loop, and moving the check against ``UINT256_MAX` does not impact specification readability.
@hwwhww hwwhww added the EIP-7594 PeerDAS label May 3, 2024
@zilm13
Copy link
Contributor

zilm13 commented May 5, 2024

Just noticed this part of code. This change will not fix the issue, because increment grows with every iteration, we could hit overflow on any step. We need something like this (but I don't like name tmp_id):

def get_custody_columns(node_id: NodeID, custody_subnet_count: uint64) -> Sequence[ColumnIndex]:
    assert custody_subnet_count <= DATA_COLUMN_SIDECAR_SUBNET_COUNT

    subnet_ids = []
    tmp_id = uint256(node_id)
    while len(subnet_ids) < custody_subnet_count:
        if tmp_id == UINT256_MAX:
            tmp_id = 0

        subnet_id = (
            bytes_to_uint64(hash(uint_to_bytes(tmp_id))[0:8])
            % DATA_COLUMN_SIDECAR_SUBNET_COUNT
        )
        if subnet_id not in subnet_ids:
            subnet_ids.append(subnet_id)
        tmp_id += 1
    assert len(subnet_ids) == len(set(subnet_ids))

    columns_per_subnet = NUMBER_OF_COLUMNS // DATA_COLUMN_SIDECAR_SUBNET_COUNT
    return sorted([
        ColumnIndex(DATA_COLUMN_SIDECAR_SUBNET_COUNT * i + subnet_id)
        for i in range(columns_per_subnet)
        for subnet_id in subnet_ids
    ])

@hwwhww hwwhww mentioned this pull request May 6, 2024
@hwwhww
Copy link
Contributor

hwwhww commented May 7, 2024

closing via #3748

@hwwhww hwwhww closed this May 7, 2024
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.

3 participants