Skip to content

chore: clarify sort for blobs#888

Merged
rootulp merged 2 commits intocelestiaorg:v0.34.x-celestiafrom
rootulp:rp/blob-sort
Nov 16, 2022
Merged

chore: clarify sort for blobs#888
rootulp merged 2 commits intocelestiaorg:v0.34.x-celestiafrom
rootulp:rp/blob-sort

Conversation

@rootulp
Copy link
Collaborator

@rootulp rootulp commented Nov 16, 2022

Partially addresses #887 by removing two exported methods which are no longer needed after this refactor: #879 (comment) and addressing this feedback: #879 (comment)

Testing

After this change, celestia-app can sort a slice of blobs via:

sort.Sort(coretypes.BlobsByNamespace(data.Blobs))

and verify a slice of blobs is sorted via:

if sort.IsSorted(coretypes.BlobsByNamespace(data.Blobs)) {
    // ...
}

@rootulp rootulp requested a review from rach-id November 16, 2022 22:13
@rootulp rootulp requested a review from evan-forbes as a code owner November 16, 2022 22:13
@rootulp rootulp self-assigned this Nov 16, 2022
@rootulp rootulp marked this pull request as draft November 16, 2022 22:14
@rootulp rootulp marked this pull request as ready for review November 16, 2022 22:15
Copy link
Member

@rach-id rach-id left a comment

Choose a reason for hiding this comment

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

Probably the tests also need to be renamed from message to blob.
example: https://github.com/rootulp/celestia-core/blob/9e33838f72574ee422f001053f64b777c4ff3893/types/block_test.go#L858.
Or should I create a separate issue for this?

@rootulp
Copy link
Collaborator Author

rootulp commented Nov 16, 2022

No need for separate issue, will keep #887 open until the rename is complete

Copy link
Member

@evan-forbes evan-forbes left a comment

Choose a reason for hiding this comment

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

🙏

@evan-forbes
Copy link
Member

why is the generated proto failing? that doesn't make any sense

Copy link
Member

@evan-forbes evan-forbes left a comment

Choose a reason for hiding this comment

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

ahh ok, we just have run make proto-gen to update some generated docs. I don't think it was from this PR tho, which is weird

@rach-id
Copy link
Member

rach-id commented Nov 16, 2022

I am having the same proto files fails on my branch. Apparently, the local version of proto is different from the CI one

@rootulp
Copy link
Collaborator Author

rootulp commented Nov 16, 2022

Looks like a test flake

--- FAIL: TestTransportHandshake (0.03s)
    transport_test.go:616: read tcp 127.0.0.1:43525->127.0.0.1:37216: i/o timeout

@rootulp rootulp merged commit 142e06b into celestiaorg:v0.34.x-celestia Nov 16, 2022
@rootulp rootulp deleted the rp/blob-sort branch November 16, 2022 23:57
evan-forbes pushed a commit that referenced this pull request Mar 11, 2025
* Add Vote Extension varying size testnet (#888)

This PR modifies the e2e app to allow varying the size of vote extensions to test the effect of VE on the system performance.
Results of tests executed are also reported here.
This work is being done as part of the v0.38.0 QA.

---

#### PR checklist

- [ ] Tests written/updated
- [ ] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog)
- [ ] Updated relevant documentation (`docs/` or `spec/`) and code comments

(cherry picked from commit 028fb5e)

# Conflicts:
#	RELEASES.md

* Fix conflict

---------

Co-authored-by: Lasaro <lasaro@informal.systems>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants