-
Notifications
You must be signed in to change notification settings - Fork 230
feat: always import inlined data in tx headers if missing #864
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
base: master
Are you sure you want to change the base?
Conversation
| ar_ignore_registry:add_ref(TXID, Ref, 5000), | ||
| post_tx_parse_id(read_body, {TXID, Req, Pid, Encoding}) | ||
| end; | ||
| case ar_mempool:is_known_tx(TXID) of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This breaks the mechanism where we process every tx only once and thus makes the network amplify spam.
We would need to do something like a two-tier ignore registry where a tx with data is accepted if it is not yet recorded in the second tier.
| tx_has_missing_data(#tx{ format = 2, data_size = DataSize, data = Data, id = ID }) | ||
| when DataSize > 0, byte_size(Data) > 0 -> | ||
| % Check if we have the data for this tx | ||
| case ar_storage:read_tx_data(ID) of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do not use the file storage anymore, use ar_data_sync:get_tx_data/1 to fetch the transaction data.
In any case, we are interested in the mempool here, we do not put unconfirmed transactions on disk. Also, we should not do any IO in the POST /tx handler for performance reasons.
I think, the ignore registry upgrade I propose above naturally solves this problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this function is now gone in favour of ignore registry
e0b55d9 to
1c6de00
Compare
1c6de00 to
cf77b61
Compare
| ar_ignore_registry:remove_ref(TXID, Ref), | ||
| ar_ignore_registry:add_temporary(TXID, 10 * 60 * 1000), | ||
| ok. | ||
| %% Exclude successful requests with valid transactions from the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
small thing: arweave codebase uses tabs for indenting
Import missing data for existing transactions
Summary
This PR allows nodes to import transaction data when posting a transaction that already exists in the mempool but is missing its data. This addresses the edge case where a format 2 transaction is initially received through gossip (without data) but later submitted with inline data that should be imported.
Problem
Currently, when a transaction already exists in the mempool, any subsequent POST to
/tximmediately returns HTTP 208 "Transaction already processed" without processing the request body. This prevents importing data for transactions that were initially received without data (e.g., through gossip).For format 2 transactions, this creates a problem:
Solution
Implemented a two-tier ignore registry system that prevents spam amplification while allowing one-time data import:
add_with_data/1andpermanent_member_with_data/1functionspost_tx_parse_id/2to check if data import should be allowed before returning 208Changes
apps/arweave/src/ar_ignore_registry.erladd_with_data/1to mark transactions as processed with datapermanent_member_with_data/1to check if transaction was already processed with dataapps/arweave/src/ar_http_iface_middleware.erlpost_tx_parse_id/2to callshould_accept_tx_with_data/2for existing transactionsshould_accept_tx_with_data/2to determine if data import should be allowed based on:handle_post_tx_accepted/3to mark format 2 transactions with data in the registryapps/arweave/test/ar_http_iface_tests.erltest_import_missing_data_for_existing_tx/1to test the edge caseKey Features
Testing
The new test case validates:
Backward Compatibility
This change is fully backward compatible: