CounterpartyXCP / counterparty-core

Counterparty Protocol Reference Implementation
http://counterparty.io
MIT License
282 stars 206 forks source link

Make `dispense` a Normal Counterparty Transaction #1670

Closed adamkrellenstein closed 3 months ago

adamkrellenstein commented 4 months ago

No longer allow a normal Bitcoin transaction to trigger a dispense; require the CNTRPRTY prefix. This will prevent users from using a vanilla Bitcoin wallet to acquire Counterparty assets, but such a wallet can't handle Counterparty assets anyway. Existing Counterparty wallets will have to use a to-be-created create_dispense API call to trigger a dispenser.

That a regular BTC transaction may trigger a dispense has major consequences for performance and code complexity because listing transactions in a block is dependent on parsing them. With this change, we will be able to fully pipeline (1) fetching blocks from bitcoind, (2) listing the Counterparty transactions in that block, and (3) parsing those transactions. We'll also be able to hard-code a list of historical dispense BTC transactions and speed up a historical catch-up.

The impact that dispensers has on node performance—as currently implemented—is really problematic. We have to call is_dispensible() for every transaction, e.g., which now have to be processed serially rather in parallel. We've now reached the limit of what we can optimize without handling the different steps of block parsing asynchronously.

301818895-dfad9db7-c6be-44a1-b922-fa63691344e7

dispenser.py is responsible for a very large fraction of the follow() process overall.

Screenshot 2024-02-02 at 7 27 38 AM

Triggering a dispenser from a non-Counterparty wallet is fundamentally unsafe, as evidenced by these warning messages:

telegram-cloud-photo-size-1-5039685767697378562-y

image

Note, this solution would also fix other existing loss-of-funds bugs in the current implementation, such as happens when users dispense multiple assets from the same dispenser. With this change, we could easily support multiple dispensers at a given address properly. We could also block creating dispense transactions when there's already one in the mempool.

Unintentional dispenses are a major problem, and have already been the justification for this patch, for instance: https://github.com/CounterpartyXCP/counterparty-core/pull/1264


Reasons that supporting triggering dispensers from a vanilla BTC wallet is not a feature:

  1. You can't just use a vanilla BTC wallet... you also have to use an explorer to find the open dispensers.
  2. The whole reason it's even desirable is that existing Counterparty wallets are unmaintained and lack critical features like UTXO management. The only mobile wallet available hasn't been update in five years.
  3. Using a vanilla bitcoin wallet is inherently unsafe, and currently the cause of loss of funds, as is demonstrated by the big red warning banner on the xchain dispenser page

Reasons that it's a bug:

  1. Triggering multiple dispensers at a single address has bizarre dynamics that cause loss of funds
  2. The current implementation is a major source of performance problems and code complexity (per above)
  3. It was not part of the original feature specification. https://github.com/CounterpartyXCP/Forum/blob/master/cip-0021.md

The concern that users may be using old wallets and not know that this change has been implemented is not real, because users need to check the list of open dispensers on an explorer first! There could be a warning posted there, just like there is a warning there now about address support in vanilla BTC wallets.

jotapea commented 4 months ago

What would be the way to "don't trust, verify" the hard-coded dispense transactions in the future? This software already includes a hard-coded list for burns which is not ideal. I believe these should be treated as optional optimizations, so that node runners that want to verify everything should be able to.

I understand the arguments to simplify the codebase, but the code is already done, so keeping it around for optional verification should not be much of a problem (then, adding the burns verification to this would be great).

Another critical point is that currently the function create_send with BTC as an asset is used to dispense. So, now requiring the create_dispense function will cause confusion or even BTC lost. And this new requirement kinda removes the "magic" of dispensers, which serve as a bridge from normal Bitcoin transactions to the Counterparty realm.

Personally, as a user of dispensers, and as a node runner, I prefer to keep them as they exist today. I was literally onboarded to Counterparty through a simple BTC send, which triggered an XCP dispense.

adamkrellenstein commented 4 months ago

What would be the way to "don't trust, verify" the hard-coded dispense transactions in the future? This software already includes a hard-coded list for burns which is not ideal. I believe these should be treated as optional optimizations, so that node runners that want to verify everything should be able to.

As I have explained offline, you just run an old version to reproduce the results.

Another critical point is that currently the function create_send with BTC as an asset is used to dispense. So, now requiring the create_dispense function will cause confusion or even BTC lost.

This change should absolutely not result in loss of funds, unless wallet operators just like never update to the new version.

And this new requirement kinda removes the "magic" of dispensers, which serve as a bridge from normal Bitcoin transactions to the Counterparty realm.

If you use a dispenser from a regular BTC wallet, you aren't able to see or use the assets you just bought from that wallet. You also risk losing funds due to lack of address support, etc.

davestaxcp commented 4 months ago

With this response I am assuming this change will cause Dispensers not to Dispense unless XCP wallets are updated with this "create_dispense" feature:

Any wallet that supports a BIP-39 seed phrase can also be imported into wallets like Freewallet (and others), despite the original wallet not showing the XCP assets.

  1. The idea that "such a wallet can't handle Counterparty assets anyway" is not a good stance when the seed-phrase itself is the wallet, not the software that is showing you the data.

You could argue which the same reasoning that 'Ordinals don't really exists on xcp addresses because those wallets can't handle ordinal assets anyway'.

  1. This feature, as originally written by Villar which stated "Dispensers just dispense tokens equivalent to fixed quantities of BTC received by an address" is used by many users especially with Electrum, which is not a Counterparty wallet at all.

Electrum supports much more Bitcoin features than all Counterparty wallets at this time. Such as RBF, CPFP and very minute UTXO and fee considerations. Which are very very important when safely buying an open Dispenser for a large sum.

  1. If this is implemented, there will be loss of funds because the community will simply use the old wallets, other wallets, or older wallet versions and complain that they didnt get their asset (and say "I got rugged, I got scammed, XCP sucks").

Even if you publicly explained this in the TG chats, Twitter, Github etc. and warned everyone... "only dispense with the new XCP wallets which are capable of doing so" you would still get a major backlash by lurking users who dont keep up often...

The user will also have to wait for these wallets to update to use this feature (if they are even paying attention).... which could cause a sudden stop of Dispenser use.... I could see the users saying "dispensers are ded" and the users will say (after losing the funds) this process simply "should not have changed since I last checked".

  1. A significant amount of published information on Dispensers would quickly be "misinformation" on how to use this process. ALOT of medium articles, twitter help threads, and deep documention has been written about how to use this feature and it is the MOST relied on feature for Counterparty assets at this time.

In fact I think reading over https://www.xcp.io/charts a Dispense can happen between 500-1000 times in a day... this suddenly changing wont go over well for people unaware

  1. There are no places users can buy these tokens on centralized exchanges and MOST users get their XCP from dispensers. This will now cause un-needed barrier of entry for new users, who already went out of their way to figure out how to use Counterparty.

  2. This is a discussion that should get the attention and feedback of the community. While I am in favor of the devs cleaning up the codebase, I am not in favor of a change that could possibly cause quick chaos amonst users, collectors and artists and incredible FUD for one the most used features on Counterparty

davestaxcp commented 4 months ago

While discussing this in Telegram, I want to make it clear - this is a very handy fix for the Rugspensers issues discussed many times on the Forums and in other places in the Github.... I am all for it going through and things to be better, but this one will need extra caution and care with the community and already published docs.

@alexliu882 mentioned in TG "how freewallet has the new version popup, it can simple have a non-closable pop up, saying an upgrade is required" - this would have to be the case for all wallets listed on: https://www.counterparty.io/wallets

And I think it would be useful to really educate users who use other non Counterparty wallets for Dispenses on social media and any other places people come to educate themselves on using Counterparty.

just my 2 sats - glad it can be discussed and people can be warned sooner rather than later

adamkrellenstein commented 4 months ago

@alexliu882 mentioned in TG "how freewallet has the new version popup, it can simple have a non-closable pop up, saying an upgrade is required" - this would have to be the case for all wallets listed on: https://www.counterparty.io/wallets

All wallets must already have something similar for the numerous protocol changes that have happened on Counterparty in the past.

ffmad commented 4 months ago

Personally, as a user of dispensers, and as a node runner, I prefer to keep them as they exist today. I was literally onboarded to Counterparty through a simple BTC send, which triggered an XCP dispense.

I think it's a bad use of Counterparty though. If you used a non-compatible XCP wallet, you might just not receive your asset. And I've had a least one experience of sending some asset to an exchange because I sent myself some bitcoin.

We should only trigger a dispenser when we want to trigger a dispenser.

This could also help building a multi-dispenser wallet @adamkrellenstein?

Instead of the very bad UX of having multiple address with 1 dispenser open, having 1 with multiple dispensers opened would be a really, really great improvement!

adamkrellenstein commented 4 months ago

This could also help building a multi-dispenser wallet @adamkrellenstein?

Yes, multi-dispenser wallets would be trivial to build with this change. (Where's it'd be impossible to make it work well with the current system.)

adamkrellenstein commented 4 months ago

Electrum supports much more Bitcoin features than all Counterparty wallets at this time. Such as RBF, CPFP and very minute UTXO and fee considerations. Which are very very important when safely buying an open Dispenser for a large sum.

This good to know!

While discussing this in Telegram, I want to make it clear - this is a very handy fix for the Rugspensers issues discussed many times on the Forums and in other places in the Github.... I am all for it going through and things to be better, but this one will need extra caution and care with the community and already published docs.

Unfortunately this proposal will not fix the "rugspensers" issue.

b1u3y commented 4 months ago

This method is shifting the dispenser logic onus to the blockchain and the user, instead of the node. Making transactions larger and increasing btc fees. What is the primary reason for the change?

Would having the node actively track and index open dispensers, while comparing new transactions against the dispenser index; have better performance than indexing all addresses with addrindexrs while helping lighten the node load and leaving the dispense tx alone?

Juans comments about how node operators should be able to have options makes sense, the code already exists.

Imagine a service where the user only needed one xcp token to pass; a new user sends BTC from an existing wallet to proceed; pass is accepted; user carries on life without looking in his 'wallet' because he doesn't care, the service he is trying to use has verified it for him. Requiring a counterparty compliant upgraded wallet for everything, while killing counterwallet, will not be ideal. A reference wallet should be available to normal users if they are to be expected to comply with the changes. Implementing UTXO based assets will face very similar headwinds.

adamkrellenstein commented 4 months ago

This method is shifting the dispenser logic onus to the blockchain and the user, instead of the node. Making transactions larger and increasing btc fees. What is the primary reason for the change?

Would having the node actively track and index open dispensers, while comparing new transactions against the dispenser index; have better performance than indexing all addresses with addrindexrs while helping lighten the node load and leaving the dispense tx alone?

None of the indexing or dispenser logic would change, but yes transactions would be marginally bigger.

jotapea commented 4 months ago

Another critical point is that currently the function create_send with BTC as an asset is used to dispense. So, now requiring the create_dispense function will cause confusion or even BTC lost. And this new requirement kinda removes the "magic" of dispensers, which serve as a bridge from normal Bitcoin transactions to the Counterparty realm.

Personally, as a user of dispensers, and as a node runner, I prefer to keep them as they exist today. I was literally onboarded to Counterparty through a simple BTC send, which triggered an XCP dispense.

I was onboarded to Counterparty through a Counterparty wallet. Using create_send with BTC as an asset, like explained. Not sure what implied this was done from a non-Counterparty wallet.

As I have explained offline, you just run an old version to reproduce the results.

This is very impractical, and against the "don't trust, verify" ethos of Bitcoin. We (at least I) am attracted to Counterparty because it IS in Bitcoin and I know how to verify things in it. Why we have hashes per block? To verify consensus of the virtual ledger of the Counterparty layer. Having hard coded stuff is so lame and UNATTRACTIVE. Adding a huge list of ledger transactions is a BIG negative to this software imo. We should lean into what aligns us more to Bitcoin, not the opposite (unless is optional).

Note, this solution would also fix other existing loss-of-funds bugs in the current implementation, such as happens when users dispense multiple assets from the same dispenser. With this change, we could easily support multiple dispensers at a given address properly.

What is the issue with the current implementation of multiple dispensers in a single address? From my understanding it is a feature, not a bug, to be able to have multiple assets being dispensed by simply sending BTC to said address.

A create_dispense function is against the spirit of dispensers. The way I see it, what would have to be done is adding a generic CNTRPRTY message to BTC create_send (could this be in cleartext?). Not sure how muti-output dispensers would be handled, maybe a new type of create_send to multiple destinations?

Finally, I would just want to add (repeat) that from my understanding, dispensers are what ONBOARDS most users to the Counterparty universe. Changes that negatively affect its UX, for some gain in node performance, may not be worth it. We have to be very careful here.

Side note: Seems like v10 is still not doing check.asset_conservation(db) (search results). I'm very curious to see how the profiling will look including this. Because we need to consider performance from 2 perspectives, the "IBD"/kickstart, and follow.

jdogresorg commented 4 months ago

I STRONGLY disagree with this change, as it adds no additional functionality, removes the ability for any bitcoin address to use a dispenser, and requires that counterparty users now use counterparty wallets…. Of which there are a few.

Also, I have not updated freewallet-mobile wallet in 4+years… and this change would make dispensers no longer work for those wallets.

From my perspective, this change needs to be discussed at length, and the pros and cons weighed…. Not just announced… as that is how NOT things operate in a community driven project . The only reason to make this change is to treat dispensers the same as other messages in counteparty, which is a desirable behavior, but in this case it comes with a cost, and that cost is too great right now.

I am supportive of you guys doing PSBT and atomic swaps and moving things in that direction.. also ok with you guys eventually depreciating dispensers when they have outlived their usefulness.... but as of right now, Dispensers are the ONE feature that has onboarded the most users to Counterparty (check Dispensers vs DEX activity), and there is no valid reason to make this change.... if there is, I have yet to see evidence beyond "dispeners bad, cause consensus issues".... and yet, Dispensers have worked for many years on Counterparty, without causing consensus issues, and been able to produce a stable ledger, for many years, before the return of the co-founders.

I am supportive of this change in the future when there are other 1-click options to buy and sell things on Counterparty (marketplace, atomic swaps, etc), but as of right now I do not feel this is an appropriate change, and feel it is being forced on the community and developers rather then focusing on improvements everyone is asking for....

AFAIK, no devs or community members are asking for any modifications to dispensers.... only to speed things up and add additional functionality, all which CAN be done without modifying dispensers.

I would be supportive of PHASING in this change, allowing both normal BTC payments to dispeners and this new create_dispense functionality to see how much usage it gets vs normal BTC payments, etc.... but, at this time, I am NOT supportive of this fundamenal change to the way dispensers operate.

jdogresorg commented 4 months ago

We could also block dispense transactions when there's already one in the mempool.

How do you envision that working since every node has a different set of transactions in mempool?

The CP Community went over some solutions to dispensers, including reserving/escrowing tokens based off transactions in the mempool... but determined that you cant rely on the mempool for consistent data, and mempool can be gamed by putting in low-fee txs, so a tx lives in mempool for a long time and most likely never gets mined, etc.

I would be interested in to see how you envision this working, and how you get around the mempool data-inconsistencies.

This change should absolutely not result in loss of funds, unless wallet operators just like never update to the new version.

This is an unfair viewpoint, as YOUR making changes to the protocol, then looking to blame wallet developers for not getting on-board with YOUR changes which broke things which were already working for many years.

Friendly reminder... node operators and developers need to be CONVINCED to upgrade to the latest release... they can not be forced! All a developer needs to do is "nothing" to cause problems and fork the ledger, so it is CRITICAL that any changes that are made during this transition period are agreed upon by the larger developer community.

My hope is that the current dev team continues to focus on optimizations and adding new features, before focusing on removing and changing already working functionality, which will result in more development required and hence, more developer pushback.

adamkrellenstein commented 4 months ago

removes the ability for any bitcoin address to use a dispenser

Nope

Also, I have not updated freewallet-mobile wallet in 4+years… and this change would make dispensers no longer work for those wallets.

LOL, yes

From my perspective, this change needs to be discussed at length, and the pros and cons weighed…. Not just announced… as that is how NOT things operate in a community driven project .

A GitHub issue isn't an announcement. That's what this little text box is for.

The only reason to make this change is to treat dispensers the same as other messages in counteparty

No, please see the OP.

there is no valid reason to make this change.... if there is, I have yet to see evidence beyond "dispeners bad, cause consensus issues"....

Have you read the issue that is under discussion?

and yet, Dispensers have worked for many years on Counterparty, without causing consensus issues, and been able to produce a stable ledger, for many years, before the return of the co-founders.

I really don't think it's necessary for me to go through and list all of the bugs that dispensers have caused. Perhaps you'll permit me to list this one, however: https://github.com/CounterpartyXCP/counterparty-core/releases/tag/v9.59.4 What is under discussion here is not removing dispensers, which you well know, but rather simply requiring additional metadata to trigger them.

AFAIK, no devs or community members are asking for any modifications to dispensers.... only to speed things up and add additional functionality, all which CAN be done without modifying dispensers.

Plenty of community members are asking for multiple dispensers at the same address to work... that's impossible with the current design.

I would be supportive of PHASING in this change, allowing both normal BTC payments to dispeners and this new create_dispense functionality to see how much usage it gets vs normal BTC payments, etc.... but, at this time, I am NOT supportive of this fundamenal change to the way dispensers operate.

See https://github.com/CounterpartyXCP/counterparty-core/issues/1784 for a discussion how this could possibly be done.

We could also block dispense transactions when there's already one in the mempool.

How do you envision that working since every node has a different set of transactions in mempool?

The CP Community went over some solutions to dispensers, including reserving/escrowing tokens based off transactions in the mempool... but determined that you cant rely on the mempool for consistent data, and mempool can be gamed by putting in low-fee txs, so a tx lives in mempool for a long time and most likely never gets mined, etc.

I would be interested in to see how you envision this working, and how you get around the mempool data-inconsistencies.

It wouldn't be consensus-critical. The mempools would not have to agree 100%. It'd be a safety valve only, but probably a pretty effective one since the Bitcoin network is very well connected.

This change should absolutely not result in loss of funds, unless wallet operators just like never update to the new version.

This is an unfair viewpoint, as YOUR making changes to the protocol, then looking to blame wallet developers for not getting on-board with YOUR changes which broke things which were already working for many years.

Why are you making this personal? All I've done is create an issue on GitHub proposing a new protocol change. I'm also not sure what changes you're referring to? This proposal is not intended to address any recent issues. Dispensers have been this way for a long time, and it has been a problem for a long time (which has gotten worse the bigger the ledger has become).

jdogresorg commented 4 months ago

I am not making it personal, i am simply objecting to the change at this time and asking for clarification on WHY this change has to be forced now.... as I know it does not need to be forced now.

I was hoping you guys would take a different route, adding improvements to CP, and then depreciating things you disagree with once better solutions are implemented.... instead, I feel your rushing forward with changes you feel are best, BEFORE implementing other alternatives (atomic swaps), and are NOT considering the counterparty community as much as you should be.

We have over a billion dollars of value on Counterparty, and EVERY change puts that value at risk.... especially changes like this that change the fundmental way that dispensers work.

I just wanted to make sure I got my objection to this change on record, before it is forced out into a release.

jotapea commented 4 months ago

Plenty of community members are asking for multiple dispensers at the same address to work... that's impossible with the current design.

Would appreciate being more specific here. I have recently bought multiple assets from a single BTC send. And it worked as expected. And I liked it.

jotapea commented 4 months ago

I was hoping you guys would take a different route, adding improvements to CP, and then depreciating things you disagree with once better solutions are implemented.... instead, I feel your rushing forward with changes you feel are best, BEFORE implementing other alternatives (atomic swaps), and are NOT considering the counterparty community as much as you should be.

+1

davestaxcp commented 4 months ago

I am supportive of you guys doing PSBT and atomic swaps and moving things in that direction.. also ok with you guys eventually depreciating dispensers when they have outlived their usefulness.... but as of right now, Dispensers are the ONE feature that has onboarded the most users to Counterparty (check Dispensers vs DEX activity), and there is no valid reason to make this change.... if there is, I have yet to see evidence beyond "dispeners bad, cause consensus issues".... and yet, Dispensers have worked for many years on Counterparty, without causing consensus issues, and been able to produce a stable ledger, for many years, before the return of the co-founders.

I am supportive of this change in the future when there are other 1-click options to buy and sell things on Counterparty (marketplace, atomic swaps, etc), but as of right now I do not feel this is an appropriate change, and feel it is being forced on the community and developers rather then focusing on improvements everyone is asking for....

I would be supportive of PHASING in this change, allowing both normal BTC payments to dispeners and this new create_dispense functionality to see how much usage it gets vs normal BTC payments, etc.... but, at this time, I am NOT supportive of this fundamenal change to the way dispensers operate.

^^^^^^^^^^^

B0BSmiths commented 4 months ago

Blocking tx creation based on mempool data can easily be circumvented by manual tx creation, and blocking will lead to two classes of dispenser users - those who can craft txs using hex / op_return and those who rely on the API

adamkrellenstein commented 3 months ago

Blocking tx creation based on mempool data can easily be circumvented by manual tx creation, and blocking will lead to two classes of dispenser users - those who can craft txs using hex / op_return and those who rely on the API

That's not what the proposal is.

adamkrellenstein commented 3 months ago

~Note: this issue is being addressed via a gradual rollout of the new, backwards-compatible TX type: https://github.com/CounterpartyXCP/counterparty-core/pull/1809~ (See below)

adamkrellenstein commented 3 months ago

@jdogresorg has threatened to fork the network even if the fully backwards-compatible proposal is implemented (even though he previously ACKed it), so there's no reason not to fix things all in one fell swoop. Recreating the issue to restart discussion of updated proposal: https://github.com/CounterpartyXCP/counterparty-core/issues/1825

adamkrellenstein commented 3 months ago

(Deleted double posts. See https://github.com/CounterpartyXCP/counterparty-core/issues/1825.)

jdogresorg commented 3 months ago

@jdogresorg has threatened to fork the network

I have made no such threat sir, I simply stated that I do not feel that changes to dispensers are necessary at this time, and I do not feel comfortable "upgrading" to this software at this time.

even though he previously ACKed it

This is incorrect, I commented on your "Alternative Dispesnser Upgrade" that I felt that the proposal could work better than the other where you immediately break mobile wallets and force users to your new "dispense" method. I did not indicate "ACK" the changes, nor say that I feel they should be included a counterparty release. Simply commented that the "alternative" path was better than the forced changes path.

I have made it clear for many months, that I disagree with changes to dispensers, and feel development should focus on PSBT and atomic swaps.

Side-Note: we now deleting comments from community members you disagree with? It costs you absolutely nothing to leave comments, and demonstrates censorship and control to delete them.... certainly doesn't feel like a decentralized community anymore 🤷‍♂

telegram-cloud-document-1-5089118173643408370

zerog975 commented 3 months ago

I think any changes that breaks existing functionality for current and past wallets should be postponed, until there are replacements in place that have been tested and working. Just because its not optimally efficient, doesn't justify rushing this out, and risking people losing funds. I still run into many users who utilize freewallet mobile fyi. This isn't a personal thing, at least for me. I don't see potential performance optimizations a fair trade off, for breaking existing functionality, without a replacement in place. Nodes are currently able keep up with the load, where is this urgency coming from? You may not like dispensers, but they are very popular in the counterparty community, and are one of the primary way artists distribute their work to collectors.

First of all, your account looks like a sock puppet. Secondly, this change will not break Freewallet mobile. :/

The proposed change does not break any existing functionality without a clear replacement workflow. All code and UX will of course be tested extensively before anything is rolled out.

This isn't about whether anyone likes dispensers or not. There are simply a number of serious flaws in the design and implementation of dispensers (which, notably, deviate from the original specification), and which it is important to close ASAP because they are causing the regular loss of funds by users. If you don't know why this change is important, read the issue that you're commenting on.

This change also is not being rushed. In the past five months of development there hasn't been a single protocol change. By contrast, there were two made by @jdogresorg in just the last few months of 2023.

Screenshot 2024-05-26 at 5 05 30 PM

Not a sock account, zerog.eth, zerog.xcp FYI, but not sure why my identity matters here.

My reading of this, would break dispensers for users of legacy clients, when purchasing from dispensers if this change is implemented. Many users don't update their wallets frequently, so this is a concern I have.

I liked Davesta's idea of a phased approach instead.