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

chore: handle PayForBlobNamespaceID in merge #1298

Merged
merged 12 commits into from
Feb 3, 2023

Conversation

rootulp
Copy link
Collaborator

@rootulp rootulp commented Jan 25, 2023

Closes #1285

Description

  1. Handle PayForBlobNamespaceID in merge
  2. Extract merge to a new file
  3. Add a unit test that uses sample block data

@rootulp rootulp self-assigned this Jan 25, 2023
@codecov-commenter
Copy link

codecov-commenter commented Jan 25, 2023

Codecov Report

Merging #1298 (373657b) into main (abfca15) will increase coverage by 0.08%.
The diff coverage is 82.85%.

❗ Current head 373657b differs from pull request most recent head b3093b7. Consider uploading reports for the commit b3093b7 to get more accurate results

@@            Coverage Diff             @@
##             main    #1298      +/-   ##
==========================================
+ Coverage   49.79%   49.87%   +0.08%     
==========================================
  Files          76       78       +2     
  Lines        4374     4383       +9     
==========================================
+ Hits         2178     2186       +8     
  Misses       2014     2014              
- Partials      182      183       +1     
Impacted Files Coverage Δ
pkg/shares/reconstruct.go 81.25% <81.25%> (ø)
pkg/shares/parse.go 82.35% <82.35%> (ø)
pkg/shares/share_sequence.go 82.35% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@evan-forbes evan-forbes added testing items that are strictly related to adding or extending test coverage share encoding labels Jan 26, 2023
@evan-forbes evan-forbes added this to the Mainnet milestone Jan 26, 2023
@rootulp rootulp requested a review from cmwaters January 30, 2023 19:48
@rootulp rootulp marked this pull request as ready for review January 30, 2023 19:48
@MSevey MSevey requested a review from a team January 30, 2023 20:02
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.

mind pointing out any changes involved in the fix? its a bit difficult to distinguish since we're moving code around.

if we wanted to get a little fancy, we could also modify an existing "fuzzing" integration test that is already generating random block data, to also test reconstruction.

@@ -0,0 +1 @@
{"header":{"version":{"block":11},"chain_id":"hVcdds","height":4,"time":"2023-01-30T18:55:27.534092Z","last_block_id":{"hash":"CnTXMWDvJXaHOjecWXb2nGvLsJ23GmgUbAYIZmteP2M=","part_set_header":{"total":1,"hash":"PgAxB6DYr8huhDZJWMnFSgYDCXwuF3jI1GNbnOskv9U="}},"last_commit_hash":"sd0o4vJyCO1W3BxOcke3/Frtu46hx+DQlDb1NjcDaTk=","data_hash":"uPWH3TVoZNb3NcyXN/vbX5N1mDZ8dfXMh9bhne04CpM=","validators_hash":"p4Iap0aX3N436a//6jkEvJV61B8xpfp82lP0P1Ji23g=","next_validators_hash":"p4Iap0aX3N436a//6jkEvJV61B8xpfp82lP0P1Ji23g=","consensus_hash":"BICRvH3cKD93v7+R1zxE2ljD34qcvIZ0Bdi389qtoi8=","app_hash":"N28hOwD/a26Nwnp3R8UteUU6TcM28zIb3+Undtt3YCw=","last_results_hash":"47DEQpj8HBSa+/TImW+5JCeuQeRkm5NMpJWZG3hSuFU=","evidence_hash":"47DEQpj8HBSa+/TImW+5JCeuQeRkm5NMpJWZG3hSuFU=","proposer_address":"XbENo47oRQwd9YSP9vr+10SttTc="},"data":{"txs":["Cp0CCn0KewoTL2Jsb2IuTXNnUGF5Rm9yQmxvYhJkCi9jZWxlc3RpYTEyc2U4MGR3aHIzOWxxNm45dXR3eXV2NDJlZnhyZXkycGV2YXludBII6P2EbA1mM94aAsEbIiDTB7FJ5XbuiAUwwKj3yEybf4eTcfZS81uO74ySllRJm0IBABJaCk4KRgofL2Nvc21vcy5jcnlwdG8uc2VjcDI1NmsxLlB1YktleRIjCiED5ZlClAcCdabMf1ivEUWgN5bJpluGNNl9XdZqlN0bNF8SBAoCCAESCBCAgOmDsd4WGkCE4EMZYENqU97tLHhxWnb5zQ64JdjqlVR2iUHxH54mrGtaThLGSKEh5ohdFQDXSHuLi+zRH7xN8XQISAhH/WLEEgEEGgRJTkRY"],"blobs":[{"namespace_id":"6P2EbA1mM94=","data":"+xg8TFSwDVXT1rILXjOb1fzQNJ6MeyO6rtOiUQCN6PIG58JsN2yFenuGREACJ31AU/TFEMzWEyj3IDLZO9NmPgRS8JCV5azfz26SmowYQtRtFcV+nICKR1HLvzuKV7Rfowey9duLvi2WST/NVaety6tRKmRbMkbljnRqlc1tFWYDPa8uOA/fSfYnDA3zH70MREnJb0GsIJ5kBlPVOVtD2o3gtD89Qwy07BF/3gAQzht7B1A0FZmV/rAjWpQ2xumLK9k2TFvgqMcltZuSy8cvxb7YHihlEPindGDch+BdZCxvYUom3bLBEcKDFnp0Cu7MeyH+299XPDzqYt7V2CLZpyV/bJUdOCPOGbNqK0mGvj1RhHz9ORrB71mGBBOAArDXUyR/GmplrhlqFQ90kyMhJR8SmPfbVgVYj/q2TyODvOagr7qb364lZcSJg1UAlwbxCJXG1xAhRnlKIkxAd3mgJ4XP6wuWOzFvyKXyc6JuusRcmm/KzyLqa7wPRAI9mRe0A9jqmY4kuJYSV7H6XqCmS6RMNeEZ/5GvpR1dWnHkLaJtCpQxQxqZdFzd8KBQqBjEsdwyZ8Wun37+ajQ6QYwcZQNjTlc8U4JQm2wWXNO3i+Z7F9fXRoy95yRS84OlBcKDxokWi+cqRsPGR2kcwF4iH8P0nQDrp7sR/sHKt01Qiw/smyCP44LHxSctU79ySmsTMm0jJSdAMfi3loC1M4S66wq/g6H2YHSOIjrAZAVqtuTt1f3C9iGfY55u3mMhwKue/4cQ/GSdAYyF25R03IPdZBG7HyCGKWkNYkj1kLszdQEJROEahx12GU/8LapzEk6c97ltd4a59d9CavbyAIpgwKseBqY2VJxl8QBPRim62WqoyOi2b6FMr7ZEMOzNxN6qnZTcysbF3mAdN7fpcrO4mrcyiDCxzWHqPxsYevcEB7vLY9EN8oPbNcc9N4nh7lfHti3HqYmCOBDEBQHjyegdjiMqRQ/mJdWzDjBf5qJiEQbMj/hXNwfIqskZHKJ3Aziv/3cz2kP3W6cifZ/KxxtrBhY8zzUoeIB36IHBiYVb1ZbsaLl+Bc8HVq1SKEt8Ykl4wyjn6LHuB+ju/sGgwWmWJUU3+9p0tzJh5rToUm0PjCa//WLvU6DZ6+5jsGyWtSoC431QPFip94LAzHM+FnlwxoWFQRo0SAzPn1MPrawj6aHAdtPX3DTSX/RR3okVIa9/0kMV4fw7rCtfp09yyeWgeRliY8dklAYQODmZMIPTteY/Y2vs6ww+KIJpdy5iEto9aaI9HmviPCmPMY666LHIY1U6LaAv3O6XRAs/dCL4olqfoMZVzUEYSmd1/1+DbzPB2lgLXg531HrQa4rQys8EfhBgwDtx0Hskh7m5bEqPx+mLMd74iOJ1WRFWt+BW44BuaWSTC4iqyBvj/t2UQBBBUK3DP/8KwrYrqKDVzVA+5Q8B2bPOEvexzS9XGoH+44hZGeI35oEvyDAisGs1ovgeeMHfjEVxGmUAGEEiIrYByFxvBqJ3fImhrhhAgabC0GR6hQgmN3qo1i4JX7cUF2IH1na9OMn4e5dwzdK0/0nAv64G4j95fv3HMQbzGyv1pkmxNDiC6fDNoWhYLclUY/jUviogffIJYSWNJK8T6kCwrtH21hnD12DBfpOFI45ESHFzgVvgY/TLQd+Xk1FoLvtiSiKHVjX2GfDOjpdhzCNhGdBVtiLRI+zX8bGHVPyDgmm7sJiQcQjSuro/nzOwcFFKkkxcPtB7Yxc8lkjfmM6tqjabMA0cjcC6jCDk4aZ8XZ5kevLSJSTfdcRbB4Z3L2FEsZ75MOrV2qF65kv5kYQ2kofQTLx63dzW6q3BejJQDf40tJAUtL3xkU2B2cqnfYuXtMQohKdGg99OAECL/QgLSCeibPDXJYGQxCzK/3h9rYdMDXGXrIAq3H0ktSQhDtXmfqto0ZIZV+i1lLZfseAfqCwuhHTakudPYTTLa9/N075ZQ2s6sToVMMTgM1y/DSlRbkfJrfX3qb+1z3EOOK/zx0O2ljzcnqD2oIbmBRpNznR7rIVOusn8KSgHJ8fkn9hHWDPAog3dYRRZWii1tg5dv9Jd7bb7U0T7SevNWRthOLSfzHn8baxp+ltH1bwBkHAHSdZhhdpdpQohAR9DZBWvHni8hVDgnPhZRS/l11BGM99UEV40obpQ/igzn7dDIEsfHbiuRERcUjXnQGSh0BXQe2eYR5ucXoUMdEtebQq3vttEdwMnjmn8rofu1Q3bO+0aSQBVOgLO1k6/G0ZL/VsFBqAuRFcY20ha/i2P6sYZLE2nXTetQqx+VwD2ttgVsqh7FzLQ+/BjM5VnobVxLa8xTUNhaaK0WRBDOSOXR3MhFP/HK3+SpXCOGYyHg3BX+G6hzSX1D7rZZdH8k6ER+sCWf6izLm4+h2pkgVXQeyrcc5ZxeLCyq24C7jgIbRL1Zrwutah2sI1bFMKOzg5bnWPSqzX6rNzwOO1Ike6YPer88rop0CyWe/NNc5i4YutmxS5/cy7eVnPMc+kKzIPhWtKv7o/soc88c3QKPcdmTyeUat1refZ+v6dzjssvH/xvfb8GFpelYyhcGBEsWCwE6WdVIlU2SShLbd7ACY50TDw8f564QjA/D9xvUDQKFNfPM+2M1uzqvjT0sTIHv6fBOtMXYAE3cetqMz3L2NnAXVWgSVw/P7Z3QXNCgZWnLLJ5OPdY769RQMc16dRvAyIPxCCoYAw4wisSz0wLPUGrpH4s3uxMNfaHKBHBGmWy8lLxGWjCrSwnCAAl1suYTMCvSP2azNncsmHswJ7GzmtkLB8etyp1flVrpzYayZH/3EYKkCoVYcs2iHqQLo3Od/OyirhwbWauHuZfdxx1er9/Roo+DIayM9DrM9TgzEMygTnvt5GBS3yinCKAbr31CiCBsF9ICankdWMqKKHO1aDrIIzfhAjcdxx/wM1mORJLm5AZmIXN3tqGKcRDxiPv7mXEkm8b068WdtH5X9qXg9PBV/caUgpEhDAuGoYpmHJrYHfWCk5++EjSDUnvP7EWNkbKFBN7jdNCZkTgPSHJb2q0B+1mUkBYROlTDp2DqO+1Th9qySZIjlCP2gZaKUo4zCtxvap5hUmD4jgzpmjBEBuKBwSZlH/0fScXazO6PWxNve1eeUrdLUJ+Zrr8A9fpZbLXTTMdflX0EPrbNU+mUBRvMz54Mfc6VtrwYd9hKiBviO1yIDcYIJQcXG3J253KSjqpIv3Uff5i5XtZNcIY1LBEdp+r5UYpwSwv9zZC/IsDZb3kggp9A1XAZZsXcDK8bd3VHH6ZrdgIDZdyDUD+8XQ2AVVHF/j7vDD59Tcs0O9yz7+WQV2qv1DlskRtaRde6SLo62H3vYM6Au/VjRT0x5Fnz/SVEGQVnav6aWEAawlCOi8xaTXQwxxqsbG+/SpX47bwQ2wECoKIg/gvfe23sgOMHVJXVfw0eA7DzreMMAnfjWVE1Ap6vLpuTGtwcUCa0Hjfp031076q9GiKrbRP5Jj/NiVV0TmMfoJFm6PQctoLdHb39ZMpthcfIsknQIzn+7jtXxxkfSww3QIBIvnG3BLBeQba4uRER5wcgUOUc+mK+YKmvMmtWbDD0Tv3+5CDONxjlFbVWsR+LNNlEf/T+2dJb00Ol2tIOd069cmkPqmvH2eNjOv/W/bubA6Ogrq9cLrAP2HNySvLp2PN1QNvAnCrvnYnwqgmYewHMrIqXiF+atJHM2qgofidLBd2W+mUsrHzrnid7NsxHGvHz5oStqTSU8e2NhOqSUUu9Hfh7Bi8p0hfXj+zIPhZfFcJFHCKc1qDN6RhmYBkpihJ90/+m2lWY2+SUgLCjse3O6ohh3bgmozyItrIFSTu1KWrdiO5mJ/snHeVKX110ZDiHku/Jygq3j8dPSF5RfElfPJxeFgSbHWX2KGtAA5oEKhp4wnhZhnFEppZ1mFrXgdPO4AAwKZ3V/vwmfbv1xZ9zT7O5PU1DymWsdQqTtR4Bwi0t0/napqH534F0cEiKT4ZVvPErTGckYfEpAuTd1JbNekrTAT3Gdy9E97LKxBGAJiP7dcfRuZKaVtt62lSkOM9dK61YYY7TlRRsG31YV04Q4eoF1Zxh81xRyV19OjIjngHzYuG7KUnmt6uMc/aQjplIEfQXud7PYm8CP4DBaqd6cAgclmsW0XkLmHk04JTe7Vd6xaPAjhK+H/2Z59tiDAYOxUqybAPy/Un8p3kf9WfkqLVzQZviOJYoIEXsQBtAA3P1f3Cs0V8NZQZcZmHCL/cCCJ6V+V3sEmhUl1nblrTgBmpfdlZRcASB20XgFXT6LQDimSYv9ycrMwpJXRxHLEgnbCo72mrBh4p2opBbfZBLkhAnkqfV/mjS/sxAZ4kKmASHM/Wbqqia2mo6FIvk9OeyVwU1v15v5iGwDd77uGB95MfcA/7qXqgPrUxaZUl9fQ8R6kcqvWIXOu8+KbSm1IB58VRxnXYFFlcRoBr4lTWHPrBivtArOSIZ6fZ/HY9Dw9oj0lACaXxZEtkLl/vDNxPEx5B8MN+onhOzgLccK5q8RBGf8hA9ebClWvRmLvYT/+yTcvOmwPWgSs6nOYprckWc47tia2350CXU6EPCnzECSE/X3AcNnwgli2Z1T+vUbYrBr3jrUjmQpujFOsuJZV9oaAfXXPlN+Lsz4I7bil4kyomQ/w3q3gTQfWmmkU="}],"square_size":4,"hash":"uPWH3TVoZNb3NcyXN/vbX5N1mDZ8dfXMh9bhne04CpM="},"evidence":{"evidence":[]},"last_commit":{"height":3,"block_id":{"hash":"CnTXMWDvJXaHOjecWXb2nGvLsJ23GmgUbAYIZmteP2M=","part_set_header":{"total":1,"hash":"PgAxB6DYr8huhDZJWMnFSgYDCXwuF3jI1GNbnOskv9U="}},"signatures":[{"block_id_flag":2,"validator_address":"XbENo47oRQwd9YSP9vr+10SttTc=","timestamp":"2023-01-30T18:55:27.534092Z","signature":"CPafxD85F5yNxKP/9hm2ew4WLPRIo472AcVNMgOZh239V4AnKsRpL8FUwS14X48DpsZuzVCqLgUKY2+agupxBQ=="}]}}
Copy link
Member

Choose a reason for hiding this comment

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

how was this testdata generated? how do we regenerate it?

admittedly, I'm not the biggest fan of using test data because the tests that rely on this could pass indefinitely, even if something in the block changes. imo, we should at least have a function to regenerate it a la https://github.com/celestiaorg/celestia-node/blob/3a1353c0d122e64b991983cf889530e2c7747d10/state/integration_test.go#L120-L147 and even then we'd still want some CI or something to notify us that it changed 😬

if we want to avoid import cycles, then I would prefer that we have those tests in a test package, but this is just a preference and doesn't have to be done in this PR

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

how was this testdata generated? how do we regenerate it?

https://github.com/celestiaorg/celestia-node/blob/3a1353c0d122e64b991983cf889530e2c7747d10/state/integration_test.go#L120-L147

how do we regenerate it?

I can add that to celestia-app

pkg/shares/reconstruct.go Outdated Show resolved Hide resolved
pkg/shares/reconstruct.go Outdated Show resolved Hide resolved
@MSevey MSevey requested a review from a team January 30, 2023 22:23
@rootulp rootulp changed the title chore: fix merge and rename to reconstruct chore: handle PayForBlobNamespaceID in merge Jan 30, 2023
@rootulp
Copy link
Collaborator Author

rootulp commented Jan 30, 2023

if we wanted to get a little fancy, we could also modify an existing "fuzzing" integration test that is already generating random block data, to also test reconstruction.

I'm concerned the existing "fuzzing" test that tests reconstruction doesn't actually generate pay for blob txs here

@evan-forbes
Copy link
Member

evan-forbes commented Jan 31, 2023

I'm concerned the existing "fuzzing" test that tests reconstruction doesn't actually generate pay for blob txs here

whoops, I should have pointed to the test I was thinking of, yeah I think we should test this using blocks that are using more realistic data, like with this test.

https://github.com/celestiaorg/celestia-app/blob/main/app/test/fuzz_abci_test.go

@evan-forbes
Copy link
Member

created a follow up for this PR #1322, in which case I think we're clear to merge

@rootulp
Copy link
Collaborator Author

rootulp commented Feb 3, 2023

Thanks for creating the follow-up issue. Will merge this as-is because it contains a fix.

@rootulp rootulp merged commit 0b447ad into celestiaorg:main Feb 3, 2023
evan-forbes pushed a commit that referenced this pull request Feb 27, 2023
Closes #1285

## Description
1. Handle PayForBlobNamespaceID in `merge`
1. Extract `merge` to a new file
1. Extract `merge` to a new file
1. Add a unit test that uses sample block data
@rootulp rootulp deleted the rp/fix-merge branch March 28, 2023 13:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
testing items that are strictly related to adding or extending test coverage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

merge doesn't handle PayForBlobNamespaceID
3 participants