oxen-io / oxen-core

Oxen core repository, containing oxend and oxen cli wallets
https://oxen.io
Other
317 stars 120 forks source link

Secure bls_rewards_request endpoint #1677

Closed Doy-lee closed 5 months ago

Doy-lee commented 5 months ago

Various fixes to ensure that bls_rewards_request sanitises all input and various bug fixes encountered during testing.

More unit tests are succeeding with this branch, there's one failing with the core fee burn which needs more investigation to figure out what is happening there.

The local-devnet python script does not work with this PR. It needs a small temporary patch to be applied. It's broken because when we transition into the Arbitrum hardfork, all the active addresses in the batching DB is still set to Oxen addresses. However the BLS rewards claim is an API designed around matching against Ethereum addresses. This causes the script to fail when trying to query/get rewards.

As denoted in the screenshot the DB contains Oxen addresses.

image

The small workaround required is the code in this patch which I've removed from this PR to keep the integration branch clean. https://github.com/oxen-io/oxen-core/pull/1677/commits/520980c0adb2182e4eb0ac3b6fc81df451657251 We require the migration code which will convert the stored addresses into Ethereum addresses.

This depends on https://github.com/oxen-io/oxen-encoding/pull/37 and https://github.com/oxen-io/oxen-core/pull/1675

darcys22 commented 5 months ago

Awesome stuff,

So something is happening, we add a node in the local devnet though the ethereum add node interface. That new node is added to our service node list and its rewards address is an ethereum address.

However its balance is no longer being added to the batching database image

darcys22 commented 5 months ago

Actually looking at that image now the service node active = false should definitely be true

darcys22 commented 5 months ago

image

Thatll be it,

contributed 100 oxen but staking requirement is 120

Doy-lee commented 5 months ago

Ah yes, some of the numbers have been changed around. I had to fix it on the unit test, should have realised that the script needs to be updated.