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

Distribute burningman role to contributors who burned bsq #6405

Conversation

HenrikJannsen
Copy link
Collaborator

@HenrikJannsen HenrikJannsen commented Nov 4, 2022

Implements bisq-network/proposals#390

Based on #6404

Starts at commit 33c08c7

@HenrikJannsen HenrikJannsen force-pushed the Distribute_Burningman_role_to_contributors_who_burned_BSQ branch from f5ee4d9 to 1e0cb12 Compare November 9, 2022 15:12
@HenrikJannsen
Copy link
Collaborator Author

Rebased on master

@HenrikJannsen HenrikJannsen marked this pull request as ready for review November 13, 2022 15:58
@HenrikJannsen
Copy link
Collaborator Author

From my side its complete now. I continue on another new PR for accounting of the received BTC but that should not have implications on that code. I will do a final in depth testing once all is complete but I tested that PR to some good extent.

Please do not merge before DAO voting has accepted the proposal.

@HenrikJannsen
Copy link
Collaborator Author

Proposal got accepted by DAO voting: bisq-network/proposals#390 (comment)

@HenrikJannsen
Copy link
Collaborator Author

Before merging please coordinate. There will be another PR and the activation data and some other data need to be finalized before merge.

@HenrikJannsen HenrikJannsen force-pushed the Distribute_Burningman_role_to_contributors_who_burned_BSQ branch from caa29ca to ba841a8 Compare November 25, 2022 21:40
core/src/main/resources/i18n/displayStrings.properties Outdated Show resolved Hide resolved
core/src/main/java/bisq/core/dao/DaoFacade.java Outdated Show resolved Hide resolved
}

// If too long for OpReturn we hash it
if (opReturnData.length > 80) {
Copy link
Member

Choose a reason for hiding this comment

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

Is 80 bytes going to be relayed?

Copy link
Collaborator Author

@HenrikJannsen HenrikJannsen Nov 27, 2022

Choose a reason for hiding this comment

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

https://www.talkcrypto.org/blog/2016/12/30/op_return-40-to-80-bytes/

https://www.oreilly.com/library/view/mastering-bitcoin/9781491902639/ch05.html

OP_RETURN was initially proposed with a limit of 80 bytes, but the limit was reduced to 40 bytes when 
the feature was released. In February 2015, in version 0.10 of Bitcoin Core, the limit was raised back to 
80 bytes. Nodes may choose not to relay or mine OP_RETURN, or only relay and mine OP_RETURN 
containing less than 80 bytes of data.

Not 100% clear if there might be issues with nodes not relaying, but seems that 80 bytes is standard limit.

Copy link
Member

Choose a reason for hiding this comment

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

I saw this comment:

https://stackoverflow.com/questions/24845429/maximum-size-of-data-bitcoin-op-return-tx-can-handle

Please note that the up to 80 part is misleading: one data push can handle up to 75 bytes. Therefore OP_RETURNs with 76-to-80 bytes need special handling: encoded with OP_PUSHDATA1 with 1-byte length. More details: bitcoin.stackexchange.com/questions/116080/… – 
Adam B
Nov 22 at 14:32

https://bitcoin.stackexchange.com/questions/116080/tx-with-op-return-with-77-bytes-of-data-refused-by-node/116083#comment132105_116083

UPDATE: The first version of this answer suggested using two data pushes, updated to use OP_PUSHDATA1.

Further research has clarified the issue:

OP_RETURN max data length is 80 bytes, however
Maximum bytes in one push data is 75 bytes.
Therefore data of sizes between 76 to 80 need to be encoded with OP_PUSHDATA1!

The script in this transaction is in fact invalid, 6a 4d 53 57 should be interpreted as OP_RETURN OP_PUSHDATA2 53 57, after which 22355 (0x5753) bytes are expected.

Correct encoding would be to use OP_PUSHDATA1 for data of length 76 to 80 bytes.

50 6a 4c 4d <77 bytes> OP_RETURN OP_PUSHDATA1 4d <77 bytes>

@HenrikJannsen HenrikJannsen force-pushed the Distribute_Burningman_role_to_contributors_who_burned_BSQ branch from 25f89e5 to bf3a360 Compare November 27, 2022 22:54
@HenrikJannsen
Copy link
Collaborator Author

Rebased on master.

.map(candidate -> new Tuple2<>(Math.round(candidate.getCappedBurnAmountShare() * spendableAmount),
candidate.getMostRecentAddress().get()))
.map(candidate -> {
double cappedBurnAmountShare = candidate.getCappedBurnAmountShare() / adjustment;
Copy link
Member

Choose a reason for hiding this comment

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

Should probably be called adjustedBurnAmountShare for clarity. It's now possible that some receivers get more than their max allowed, although only by a few satoshis.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, I think it does not really add a risk as its only small amounts

}

// If too long for OpReturn we hash it
if (opReturnData.length > 80) {
Copy link
Member

Choose a reason for hiding this comment

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

I saw this comment:

https://stackoverflow.com/questions/24845429/maximum-size-of-data-bitcoin-op-return-tx-can-handle

Please note that the up to 80 part is misleading: one data push can handle up to 75 bytes. Therefore OP_RETURNs with 76-to-80 bytes need special handling: encoded with OP_PUSHDATA1 with 1-byte length. More details: bitcoin.stackexchange.com/questions/116080/… – 
Adam B
Nov 22 at 14:32

https://bitcoin.stackexchange.com/questions/116080/tx-with-op-return-with-77-bytes-of-data-refused-by-node/116083#comment132105_116083

UPDATE: The first version of this answer suggested using two data pushes, updated to use OP_PUSHDATA1.

Further research has clarified the issue:

OP_RETURN max data length is 80 bytes, however
Maximum bytes in one push data is 75 bytes.
Therefore data of sizes between 76 to 80 need to be encoded with OP_PUSHDATA1!

The script in this transaction is in fact invalid, 6a 4d 53 57 should be interpreted as OP_RETURN OP_PUSHDATA2 53 57, after which 22355 (0x5753) bytes are expected.

Correct encoding would be to use OP_PUSHDATA1 for data of length 76 to 80 bytes.

50 6a 4c 4d <77 bytes> OP_RETURN OP_PUSHDATA1 4d <77 bytes>

@HenrikJannsen
Copy link
Collaborator Author

Ok, I reduce the opReturn to 40 bytes. Its anyway just a new feature without concrete use case so far and an only UI thing.
The opReturn size limit issue got heavily mis-managed by the Bitcoin community. Would not like to know how many dev hours have been wasted for that...

@HenrikJannsen HenrikJannsen force-pushed the Distribute_Burningman_role_to_contributors_who_burned_BSQ branch from 47a3906 to b25c25c Compare November 29, 2022 15:16
@HenrikJannsen
Copy link
Collaborator Author

Rebased on master

Copy link
Member

@sqrrm sqrrm left a comment

Choose a reason for hiding this comment

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

No more comments at the moment, will have to test it but code wise I think it looks good.

@ripcurlx ripcurlx added this to the v1.9.7 milestone Dec 1, 2022
@ghost
Copy link

ghost commented Dec 2, 2022

In regtest, the first person to burn is has a suggested amount in the range of a few thousand BSQ. Thereafter, burn targets are much smaller than what the first BM burned. e.g. Alice burns 2000 BSQ. All other BMs will only be allowed to burn 220 BSQ. Soon after, no one can burn anything, even if they go through several cycles of compensation.
It does not seem right, and maybe its because I'm missing some setup in regtest. Could it be due to there not being any proof of burns from the legacy BM? How would I go about simulating burns from the legacy BM, address 2MzBNTJDjjXgViKBGnatDU3yWkJ8pJkEg9w ?

// interferes with usage of the burningManCandidatesByName map.
// Those are the hashes used by legacy BM for burning BTC received from DPT.
Set<String> hashes = Set.of("1701e47e5d8030f444c182b5e243871ebbaeadb5e82f",
"1701293c488822f98e70e047012f46f5f1647f37deb7");
Copy link

Choose a reason for hiding this comment

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

Why are these burningman hashes hard-coded?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Those are the hashes in the opReturn at burn txs of the BM to tag different use cases (btc fee, del payout tx to refund agent, del payout tx to trader)

// interferes with usage of the burningManCandidatesByName map.
// The hash used by legacy BM for burning BTC received from BTC trade fees.
Set<String> hashes = Set.of("1701721206fe6b40777763de1c741f4fd2706d94775d");
LegacyBurningMan legacyBurningManBtcFees = getLegacyBurningMan(LEGACY_BURNING_MAN_BTC_FEES_ADDRESS, hashes);
Copy link

Choose a reason for hiding this comment

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

Same again, hardcoded? I have to replace it with the relevant hashes from my database in order to pick up the legacy burningman's activity.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You mean for testing it with regtest?

Copy link

Choose a reason for hiding this comment

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

Yes, Regtest. I'm confused as to the necessity for hardcoding those hashes: if BM3 does a fee burn, I would have expected it to get integrated in real time, instead of pasting into the sourcecode.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The burn tx opReturn data are arbitrary bytes. One can add some preimage which gets hashed in the UI, and that is what the BM is doing to tag the diff. burn txs to groups. So this helps us to filter for burn txs from btc fees and those for DPTs.

@HenrikJannsen HenrikJannsen force-pushed the Distribute_Burningman_role_to_contributors_who_burned_BSQ branch from b25c25c to dd684a4 Compare December 4, 2022 18:18
We will use that for the optional address field in compensation requests and interpret empty input as Optional.empty.

Signed-off-by: HenrikJannsen <[email protected]>
We cannot add a new field as that would break DAO consensus.

Add optional text field for burningManReceiverAddress to CompensationProposal UI.

Signed-off-by: HenrikJannsen <[email protected]>
Will be used later by BurningMan domain.

Signed-off-by: HenrikJannsen <[email protected]>
Signed-off-by: HenrikJannsen <[email protected]>
…ill be used later)

Change return type of FormBuilder.addTableViewWithHeader

Signed-off-by: HenrikJannsen <[email protected]>
Signed-off-by: HenrikJannsen <[email protected]>
…utsForDepositTxRequest and ProcessModel.

Use BurningManService.getBtcFeeReceiverAddress at createFeeTx tasks.
Verify takers burningManSelectionHeight if BurningManService.isActivated at MakerProcessesInputsForDepositTxRequest and OpenOfferManager.
Stores burningManSelectionHeight in ProcessModel.
Add BurningManService fields to OfferAvailabilityModel, PlaceOfferModel, OpenOfferManager and Provider

Signed-off-by: HenrikJannsen <[email protected]>
Add getCycleAtIndex convenience method to DaoStateService

Signed-off-by: HenrikJannsen <[email protected]>
A cycle is the fundamental unit in the DAO, months/years have no relevance, so using
cycles as base helps avoids some potential weird behaviour with data getting updated at cycle events.

Signed-off-by: HenrikJannsen <[email protected]>
Signed-off-by: HenrikJannsen <[email protected]>
Signed-off-by: HenrikJannsen <[email protected]>
Signed-off-by: HenrikJannsen <[email protected]>
… 100% from capped burn shares.

Signed-off-by: HenrikJannsen <[email protected]>
For chainHeights < genesisHeight+3*grid we ended up with future chainHeights

Signed-off-by: HenrikJannsen <[email protected]>
It is not included in BurningManCandidate candidate map as that would complicate things.
We only want to show it in the UI for informational purposes.

Signed-off-by: HenrikJannsen <[email protected]>
…er fee in DelayedPayoutTxReceiverService.getSpendableAmount

Signed-off-by: HenrikJannsen <[email protected]>
Signed-off-by: HenrikJannsen <[email protected]>
Signed-off-by: HenrikJannsen <[email protected]>
Remove sorting as not needed.

Signed-off-by: HenrikJannsen <[email protected]>
Signed-off-by: HenrikJannsen <[email protected]>
… some outputs fall below the min. amount limit.

Signed-off-by: HenrikJannsen <[email protected]>
Signed-off-by: HenrikJannsen <[email protected]>
@HenrikJannsen HenrikJannsen force-pushed the Distribute_Burningman_role_to_contributors_who_burned_BSQ branch from dd684a4 to a17eb60 Compare December 9, 2022 14:59
Signed-off-by: HenrikJannsen <[email protected]>
@HenrikJannsen
Copy link
Collaborator Author

Close that as #6423 is th final PR and based on that.

@ripcurlx ripcurlx removed this from the v1.9.7 milestone Dec 12, 2022
@HenrikJannsen HenrikJannsen deleted the Distribute_Burningman_role_to_contributors_who_burned_BSQ branch December 29, 2022 17:50
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