fix(bmr): send only one receipt per transaction #545

MuhammedIrfan · 2022-09-26T11:52:15Z

Closes #543
Closes #186

Checklist:

I have performed a self-review of my own code
I have documented my code in accordance with the documentation guidelines
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
I have run the unit tests
I only have one commit (if not, squash them into one commit).
I have a descriptive commit message that adheres to the commit message guidelines

codecov · 2022-09-26T12:04:38Z

Codecov Report

Merging #545 (bf923b0) into main (4e691bb) will increase coverage by 0.49%.
The diff coverage is 77.27%.

@@             Coverage Diff              @@
##               main     #545      +/-   ##
============================================
+ Coverage     42.01%   42.51%   +0.49%     
  Complexity      801      801              
============================================
  Files           115      116       +1     
  Lines         11321    11317       -4     
  Branches        941      941              
============================================
+ Hits           4757     4811      +54     
+ Misses         6155     6093      -62     
- Partials        409      413       +4

Impacted Files	Coverage Δ
cmd/iconbridge/chain/icon/sender.go	`0.00% <0.00%> (ø)`
cmd/iconbridge/chain/bsc/sender.go	`33.85% <50.00%> (-4.86%)`	⬇️
cmd/iconbridge/chain/near/sender.go	`61.49% <66.66%> (+14.00%)`	⬆️
cmd/iconbridge/common/chainutils/segment.go	`89.28% <89.28%> (ø)`
cmd/iconbridge/chain/bsc/verifier.go	`47.05% <0.00%> (-0.66%)`	⬇️
cmd/iconbridge/chain/substrate-eth/receiver.go	`62.77% <0.00%> (ø)`
cmd/iconbridge/chain/bsc/receiver.go	`59.02% <0.00%> (+1.42%)`	⬆️

cmd/iconbridge/chain/near/sender.go

bbist · 2022-09-27T09:31:34Z

Hi @MuhammedIrfan,
Can you try with the latest changes? You need to provide a configurable value for tx_data_size_limit from the config file as specified here.

CyrusVorwald · 2022-09-27T12:32:12Z

These changes need tests.

bbist · 2022-09-27T13:51:41Z

These changes need tests.

Thanks @CyrusVorwald-ICON for pointing it out. It wasn't a final solution, but rather an intermediate one to see if it works for NEAR. Anyway, I'll refactor the shared code for segmentation used across all chains and add necessary test and push again.

cmd/iconbridge/common/chainutils/segment.go

…paid-gas-error' of github.com:icon-project/icon-bridge into fix/543-bmc-transaction-failing-due-to-exceeded-the-prepaid-gas-error

bbist · 2022-09-30T06:43:36Z

Hi @MuhammedIrfan,

The tests that were failing on near side were not implemented correctly. I've changed the test parameters so that they work as expected. They should work now.

bbist · 2022-09-30T06:45:04Z

Hi @MuhammedIrfan,

The tests that were failing on near side were not implemented correctly. I've changed the test parameters so that they work as expected. They should work now.

Btw, have you deployed this code? Let's test it on local and see if it resolves the original issue that this PR resolves. If it does, I'll approve the changes and merge into main.

MuhammedIrfan · 2022-10-07T11:27:23Z

@bbist looks like it does not add message to tx if rlp size is more than configured limit also if it have only 1 event

bbist · 2022-10-07T12:35:40Z

@bbist looks like it does not add message to tx if rlp size is more than configured limit also if it have only 1 event

Yeah. That's true. You can configure a limit that can handle 1 event. We did the same for BSC.

MuhammedIrfan · 2022-10-07T12:41:06Z

@bbist looks like it does not add message to tx if rlp size is more than configured limit also if it have only 1 event

Yeah. That's true. You can configure a limit that can handle 1 event. We did the same for BSC.

But we can't always be sure of size right, at times when the event size is more than configured it create relay message without any receipts eg: https://explorer.testnet.near.org/transactions/3or4kyeL7fQ6uds73BfCynMbHscnBG4tLjwCHNA4EJon

…ded-the-prepaid-gas-error

bbist · 2022-10-19T08:00:34Z

@bbist looks like it does not add message to tx if rlp size is more than configured limit also if it have only 1 event

Yeah. That's true. You can configure a limit that can handle 1 event. We did the same for BSC.

But we can't always be sure of size right, at times when the event size is more than configured it create relay message without any receipts eg: https://explorer.testnet.near.org/transactions/3or4kyeL7fQ6uds73BfCynMbHscnBG4tLjwCHNA4EJon

What is the maximum number of events the NEAR chain can handle in a single transaction? Is it a single event of can it handle 2-3 or more? If so, you can chose some data size limit that can cover 2 or more events. And in the worst case, it'll forward 1 event. This is what we did for BSC.
If this approach is sufficient, then it's fine. Otherwise, we might have to modify the code to also support for minimum number of events, rather than the transaction data size limit.

…ded-the-prepaid-gas-error

MuhammedIrfan · 2022-10-22T03:47:45Z

@bbist

in the worst case, it'll forward 1 event. This is what we did for BSC. If this approach is sufficient, then it's fine. Otherwise, we might have to modify the code to also support for minimum number of events, rather than the transaction data size limit.

This is not happening, I have updated the test to test this scenario. Please check.

What is the maximum number of events the NEAR chain can handle in a single transaction? Is it a single event of can it handle 2-3 or more?

At present NEAR supports only one event which is being improved as part of gas optimization, As different BTP Messages have different gas usage, for example, Init BTP Message will use less than that transfer, and Link BTP Message uses more gas, Until we find a way to optimize and segment the message in the relay we might just keep only 1 BTP Message per Relay Message

bbist · 2022-10-22T04:23:52Z

@bbist

in the worst case, it'll forward 1 event. This is what we did for BSC. If this approach is sufficient, then it's fine. Otherwise, we might have to modify the code to also support for minimum number of events, rather than the transaction data size limit.

This is not happening, I have updated the test to test this scenario. Please check.

What is the maximum number of events the NEAR chain can handle in a single transaction? Is it a single event of can it handle 2-3 or more?

At present NEAR supports only one event which is being improved as part of gas optimization, As different BTP Messages have different gas usage, for example, Init BTP Message will use less than that transfer, and Link BTP Message uses more gas, Until we find a way to optimize and segment the message in the relay we might just keep only 1 BTP Message per Relay Message

Well, in that case as a temporary solution, let's have a minimum number of events set to 1 while segmenting for NEAR.

bbist · 2022-11-02T13:45:15Z

@bbist

in the worst case, it'll forward 1 event. This is what we did for BSC. If this approach is sufficient, then it's fine. Otherwise, we might have to modify the code to also support for minimum number of events, rather than the transaction data size limit.

This is not happening, I have updated the test to test this scenario. Please check.

What is the maximum number of events the NEAR chain can handle in a single transaction? Is it a single event of can it handle 2-3 or more?

At present NEAR supports only one event which is being improved as part of gas optimization, As different BTP Messages have different gas usage, for example, Init BTP Message will use less than that transfer, and Link BTP Message uses more gas, Until we find a way to optimize and segment the message in the relay we might just keep only 1 BTP Message per Relay Message

Well, in that case as a temporary solution, let's have a minimum number of events set to 1 while segmenting for NEAR.

@MuhammedIrfan,
Has necessary changes been made on this issue? Please don't let it be a blocker for the integration. You can make necessary changes in your NEAR specific Segment() function to forward a single event, and maybe add a TODO comment to comeback to this issue later. Let's try to resolve this PR.

andrii-kl · 2022-11-07T12:48:31Z

cmd/iconbridge/chain/near/tests/sender_segment.go

+				Message     *chain.Message
+				Options     types.SenderOptions
+			}{
+				PrivateKey:  "22yx6AjQgG1jGuAmPuEwLnVKFnuq5LU23dbU3JBZodKxrJ8dmmqpDZKtRSfiU4F8UQmv1RiZSrjWhQMQC3ye7M1J",


It would be good to generate a random one instead predefined for private key

fix(bmr): send only one receipt per transaction

7bf75fb

MuhammedIrfan requested a review from bbist September 26, 2022 11:52

MuhammedIrfan linked an issue Sep 26, 2022 that may be closed by this pull request

bug(bmc): transaction failing due to Exceeded the prepaid gas. error #543

Open

MuhammedIrfan marked this pull request as ready for review September 26, 2022 11:57

MuhammedIrfan requested review from manishbista28 and andrii-kl as code owners September 26, 2022 11:57

bbist requested changes Sep 26, 2022

View reviewed changes

cmd/iconbridge/chain/near/sender.go Outdated Show resolved Hide resolved

cmd/iconbridge/chain/near/sender.go Show resolved Hide resolved

fix tx size limit exceeding when single receipt has too many events

1915df5

bbist self-requested a review September 27, 2022 09:29

bbist added 2 commits September 27, 2022 20:15

move shared segementation code to separate package

a1af6ab

add unit tests for segmentation by tx data size

f824d50

bbist self-assigned this Sep 27, 2022

MuhammedIrfan commented Sep 28, 2022

View reviewed changes

cmd/iconbridge/common/chainutils/segment.go Show resolved Hide resolved

MuhammedIrfan and others added 5 commits September 28, 2022 17:46

add more tests

c926648

copy receipt to avoid input receipts mutation

6eeecbd

Merge branch 'fix/543-bmc-transaction-failing-due-to-exceeded-the-pre…

62dacce

…paid-gas-error' of github.com:icon-project/icon-bridge into fix/543-bmc-transaction-failing-due-to-exceeded-the-prepaid-gas-error

skip test with external rpc call

0dcaff4

resolve the cause of failing unit tests

49446d2

Merge branch 'main' into fix/543-bmc-transaction-failing-due-to-excee…

77afbd7

…ded-the-prepaid-gas-error

MuhammedIrfan added the team: hugobyte Relevant to HugoByte label Oct 7, 2022

Merge branch 'main' into fix/543-bmc-transaction-failing-due-to-excee…

c492fac

…ded-the-prepaid-gas-error

MuhammedIrfan added 2 commits October 20, 2022 01:28

Merge branch 'main' into fix/543-bmc-transaction-failing-due-to-excee…

63492df

…ded-the-prepaid-gas-error

update testcase to assert size of relay message

d847797

MuhammedIrfan added 2 commits October 22, 2022 09:21

fix missing param for e2e test

099aff7

fix testcase assertions

bf923b0

andrii-kl approved these changes Nov 7, 2022

View reviewed changes

andrii-kl self-requested a review November 7, 2022 12:51

MuhammedIrfan marked this pull request as draft November 18, 2022 13:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(bmr): send only one receipt per transaction #545

fix(bmr): send only one receipt per transaction #545

MuhammedIrfan commented Sep 26, 2022 •

edited

Loading

codecov bot commented Sep 26, 2022 •

edited

Loading

bbist commented Sep 27, 2022 •

edited

Loading

CyrusVorwald commented Sep 27, 2022

bbist commented Sep 27, 2022 •

edited

Loading

bbist commented Sep 30, 2022 •

edited

Loading

bbist commented Sep 30, 2022

MuhammedIrfan commented Oct 7, 2022

bbist commented Oct 7, 2022

MuhammedIrfan commented Oct 7, 2022

bbist commented Oct 19, 2022

MuhammedIrfan commented Oct 22, 2022 •

edited

Loading

bbist commented Oct 22, 2022

bbist commented Nov 2, 2022

andrii-kl Nov 7, 2022

fix(bmr): send only one receipt per transaction #545

Are you sure you want to change the base?

fix(bmr): send only one receipt per transaction #545

Conversation

MuhammedIrfan commented Sep 26, 2022 • edited Loading

Checklist:

codecov bot commented Sep 26, 2022 • edited Loading

Codecov Report

bbist commented Sep 27, 2022 • edited Loading

CyrusVorwald commented Sep 27, 2022

bbist commented Sep 27, 2022 • edited Loading

bbist commented Sep 30, 2022 • edited Loading

bbist commented Sep 30, 2022

MuhammedIrfan commented Oct 7, 2022

bbist commented Oct 7, 2022

MuhammedIrfan commented Oct 7, 2022

bbist commented Oct 19, 2022

MuhammedIrfan commented Oct 22, 2022 • edited Loading

bbist commented Oct 22, 2022

bbist commented Nov 2, 2022

andrii-kl Nov 7, 2022

Choose a reason for hiding this comment

MuhammedIrfan commented Sep 26, 2022 •

edited

Loading

codecov bot commented Sep 26, 2022 •

edited

Loading

bbist commented Sep 27, 2022 •

edited

Loading

bbist commented Sep 27, 2022 •

edited

Loading

bbist commented Sep 30, 2022 •

edited

Loading

MuhammedIrfan commented Oct 22, 2022 •

edited

Loading