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

Implement dispense Transaction Type #1825

Open
3 tasks
adamkrellenstein opened this issue May 26, 2024 · 17 comments
Open
3 tasks

Implement dispense Transaction Type #1825

adamkrellenstein opened this issue May 26, 2024 · 17 comments

Comments

@adamkrellenstein
Copy link
Member

It is a serious flaw in the protocol that dispenses are not normal Counterparty transactions. It was also not part of the original specification. A non-exhaustive list of the ways in which this causes major problems may be found here: #1670 (comment)

We need to:

  • Prevent vanilla BTC sends from triggering dispensers, which is unsafe and causes loss of funds on a regular basis
  • Create a new dispense TX type that is a normal Counterparty transaction (with the CNTRPRTY prefix) that specifies the asset requested and the quantity (to eliminate accidental dispenses from multiple dispensers registered at the same address)
  • Modify the compose_send() API call to infer whether the user is trying to trigger a dispenser—if so, generate a dispense transaction in the background (which will obv. also be a valid BTC send). Allow dispenser hosts to choose the default dispenser; if none is specified, use the first (and purchase the maximum possible).
@zerog975
Copy link

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 proven to work.
Just because its not optimally efficient, doesn't justify rushing this out, and risk people losing funds.
I still run into many users who still utilize freewallet mobile.
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.

@adamkrellenstein
Copy link
Member Author

adamkrellenstein commented May 26, 2024

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 proven to work. Just because its not optimally efficient, doesn't justify rushing this out, and risk people losing funds. I still run into many users who still utilize freewallet mobile. 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. Second, do not double post. Third, 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 in just the last few months of 2023.

Screenshot 2024-05-26 at 5 05 30 PM

@zerog975
Copy link

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 proven to work. Just because its not optimally efficient, doesn't justify rushing this out, and risk people losing funds. I still run into many users who still utilize freewallet mobile. 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. Second, do not double post. Third, 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 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.

Breaking changes should be treated seriously. Users will have the real potential to lose funds, by continuing to use counterparty in the way they have for years

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

@davestaxcp
Copy link

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

The only idea I am supportive of is to implement the proposed changes in the XCP whitepaper BEFORE changing dispensers.

In fact, the alternative path for upgrading dispensers in a slower manner over a longer period of time (that doesnt effect existing wallets) was not created by me, but created by Adam and mentioned in #1784

If my opinion even matters, I simply would like to see users have something else they can use to buy/sell XCP tokens before changing dispensers.... which wasn't even mentioned in the whitepaper at all

Users dont consider dispensers as a bug or flaw, despite them not being perfect.

@zerog975
Copy link

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 proven to work. Just because its not optimally efficient, doesn't justify rushing this out, and risk people losing funds. I still run into many users who still utilize freewallet mobile. 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. Second, do not double post. Third, 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 in just the last few months of 2023.

Screenshot 2024-05-26 at 5 05 30 PM

I won't double post, if my posts aren't removed.

@pinnate-modo
Copy link

pinnate-modo commented May 27, 2024

  1. atomic swaps will not replace any functionality that will be removed with this change (i.e. triggering dispenses from non-Counterparty wallets); they're completely independent.
  2. adding features before fixing bugs is an objectively bad development practice (and the state of Counterparty's codebase is a pretty damning testament to that fact).
  3. a 'phase-in' upgrade path was designed to minimize the impact on freewallet users (other wallet providers have already said they're fine with the upgrade). @jdogresorg agreed to it... then rejected it, so phasing the change in will not help.
  4. no one can force users to upgrade their software, but if you give reasonable notice (and we're talking about > 2 months of notice here) that quite simply cannot block development in perpetuity.
  5. people are currently losing money completely needlessly in huge quantities because of accidental dispenses, the fact that you can send BTC to an empty dispenser, sending from taproot or p2wsh addresses, and so on---all of which will be addressed with this change. I do not understand why those losses of funds, which result from bugs in the software, should be discounted out of concern for future potential losses of funds by those who don't upgrade their software.

@adamkrellenstein
Copy link
Member Author

adamkrellenstein commented May 27, 2024

The only idea I am supportive of is to implement the proposed changes in the XCP whitepaper BEFORE changing dispensers.

In fact, the alternative path for upgrading dispensers in a slower manner over a longer period of time (that doesnt effect existing wallets) was not created by me, but created by Adam and mentioned in #1784

If my opinion even matters, I simply would like to see users have something else they can use to buy/sell XCP tokens before changing dispensers.... which wasn't even mentioned in the whitepaper at all

Users dont consider dispensers as a bug or flaw, despite them not being perfect.

Thank you for the reasoned and sober comment. My perspective is as follows:

  1. Critical bugs like these generally need to be fixed before implementing significant new functionality—and especially in this case because the dispensers code makes a huge mess of all of the block and transaction parsing logic. Simply making dispenses normal Counterparty transactions will massively simplify the codebase and pave the way for implementing relatively complex upgrades like atomic swaps.

  2. I consider these changes to dispensers to be pretty minor. Existing Counterparty wallet functionality should be extremely similar (multi-asset dispensers will be the only the only things affected); users will really just no longer be able to use vanilla Bitcoin wallets to trigger dispensers, but that's already a very risky endeavor, and they will be able to be warned effectively not to do so by the block explorer that they find the dispensers on in the first place.

  3. It's true that members of the community generally seem to be okay with the way things are right now, but I expect that's a lot of survivor bias. I can't imagine how many users Counterparty has scared away because someone accidentally triggered a dispenser, or because they used a taproot address, etc. and lost thousands of dollars because the current design is a huge foot-gun.

  4. As far as I'm concerned, this is a gradual approach to solving this problem. We're making the Counterparty node API backwards-compatible, and we're tackling these (painfully obvious) problems only after five full months of heavy development (resolving, I don't know 200 issues??) comprising exactly zero protocol changes. The easiest thing would absolutely to have been to fix these problems first, because of how much more difficult AddrIndexRs and the dispenser logic make the codebase to work on. (I'd say a full 50% of all bugs / regressions that we've found in the past five months have been related to this stuff. That is, development has been ~50% slower than it otherwise would have been.) Even still, I wanted to prioritize refactoring, cleanup and testing, because they're not user-facing and because it was the more prudent thing to do.

@jdogresorg
Copy link
Contributor

jdogresorg commented May 27, 2024

  1. a 'phase-in' upgrade path was designed to minimize the impact on freewallet users (other wallet providers have already said they're fine with the upgrade).
  1. Welcome to counterparty @pinnate-modo! It looks like your relatively new, as is your github, and you have only interacted with counterparty repo in the past few months, and only to support core dev work... so, as Adam has said "your account looks like a sock puppet."

  2. You indicate "other wallet providers have already said they're fine with the upgrade". Awesome! Whom? What other open-source wallets? AFAIK there are no open-source counterparty wallets except FreeWallet, and no other wallets support all the functionality Counterparty CURRENTLY has Multi-Peer-Multi-Asset (MPMA) Sends, Dispensers, Orders, etc..

@jdogresorg agreed to it... then rejected it, so phasing the change in will not help.

let me once again, address this false accusation that I changed my mind or am threatening a fork. I have already addressed this falsehood multiple times, in multiple github issues, but the current core devs seem to keep closing issues with differing views and reopening new issues to rehash existing conversations.

As you can see, I clearly addressed this falsehood already.

Github Post #1670
Github Post #1784
Github Post #1809


@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

I am no desire to engage on this issue any further, but will continue to speak up LOUDLY to defend myself, anytime someone accuses me of something false. 🤷‍♂️

@zerog975
Copy link

zerog975 commented May 27, 2024

Since this is discussing a protocol level change to dispensers, I think its important that the design should attempt to fix what I think is one of the biggest issues with dispensers currently, as a user.
Since this is a protocol level change, I think it should fix as much as possible in one go.

Rugspensers, where the seller and setup a dispenser, and once another user puts in a buy in the mempool, he buys it himself with a higher fee.

Here are some previous discussions
https://forums.counterparty.io/t/solutions-to-rugspenser-problem/6623
CounterpartyXCP/Forum#120
CounterpartyXCP/Forum#121

What about something like a op_return dispense_reserve function, that buyers could use to reserve the dispenser for a number of blocks, like 10?
Interested in any other solutions as well.

@loon3
Copy link

loon3 commented May 27, 2024

Since this is discussing a protocol level change to dispensers, I think its important that the design should attempt to fix what I think is one of the biggest issues with dispensers currently, as a user. Since this is a protocol level change, I think it should fix as much as possible in one go.

Rugspensers, where the seller and setup a dispenser, and once another user puts in a buy in the mempool, he buys it himself with a higher fee.

Here are some previous discussions https://forums.counterparty.io/t/solutions-to-rugspenser-problem/6623 CounterpartyXCP/Forum#120 CounterpartyXCP/Forum#121

What about something like a op_return dispense_reserve function, that buyers could use to reserve the dispenser for a number of blocks, like 10? Interested in any other solutions as well.

this is the nature of dispensers, they have this flaw by design, if you want to remove this then just use a btcpay tx and you have 20 blocks to pay from when the order is matched

@b1u3y
Copy link

b1u3y commented May 28, 2024

What about something like a op_return dispense_reserve function, that buyers could use to reserve the dispenser for a number of blocks, like 10?

This would inadvertently create an 'options market'.

@mikeinspace
Copy link

As someone who has inadvertently (and stupidly) dispensed tokens from dispensers without intent on a number of occasions, I think this solution, on balance, is warranted. People have the potential to lose money in both scenarios (presently) or with these changes (accidently using a non cp-aware wallet), so I'm not really swayed by appeals to keep things the same as a means to prevent losses.

I do think a lot of the potential for losses can be mitigated with an educational campaign around the new behaviour and labelling explorers very clearly that they should be using a cp-aware wallet (and linking to one) right next to a dispenser's QR code.

Perhaps even an interstitial that hides the QR code by default with messaging that you must "click to acknowledge" before revealing the QR code can help mitigate this issue.

So, on balance, I think this change is warranted.

@jotapea
Copy link
Contributor

jotapea commented Jun 28, 2024

Something like #1814 would solve the accidental dispenses. I just don't agree over-engineering the feature is the best solution here. I believe less is more in this one.

@droplister
Copy link
Contributor

droplister commented Jul 11, 2024

I agree that the way dispensers were implemented wasn't ideal. They have been successful at lowering the barrier to entry. And have created a source of BTC buy-side liquidity that was not present without CEX listings.

I'm not sure accidental dispenses has been a major issue, people have used this much more as a feature to have dispensers that give multiple assets for one payment. See: 1BigDeaLFejyJiK6rLaj4LYCikD5CfYyhp

This change wouldn't prevent the risk of someone self-closing or sniping a dispenser from you in the mempool resulting in a loss of funds. It probably adds a new way to lose funds and that's sending BTC without including the dispense message.

Opening on addresses you don't prove you own has been a nice way to incentivize donations where people are accepting BTC, for example. And I've used it extensively to create dispensers that would land funds in my cold wallet.

The state of things for years has been you send BTC and you get dispensed what's open and at that address for a price at or below the amount of BTC you've sent. For the users not following along with these changes, I'd expect an increase in loss of funds due to people doing what they're familiar with and sending the BTC without the dispense message.

Getting rid of AddrIndex makes sense to me. Making dispenses follow the normal conventions of Counterparty makes sense to in the abstract, but it's a little bit what's done is done in my view.

UTXO binding to allow for PSBTs looks like the ultimate, alternate solution to dispensers that would allow for the AddrIndex removal and sunsetting of dispensers.

Is what happens to existing, open dispensers after these changes still an open question? I'm imagining it would like an order expiration except for a whole lot of assets at once.

@droplister
Copy link
Contributor

droplister commented Jul 11, 2024

I think PSBTs are better than dispensers. The benefit of this change is primarily the removal of AddrIndex. I wouldn't say dispensers are made safer after these changes, but they're not made considerably worse if wallets exist to adopt this change. If dropping AddrIndex streamlines things and keeps development velocity high towards a UTXO-based option, it will be worth it.

tl;dr - Overall, I support this change.

@loon3
Copy link

loon3 commented Jul 11, 2024

It probably adds a new way to lose funds and that's sending BTC without including the dispense message.

This is probably the biggest risk with this update but I don't think its a showstopper. Important risk to understand and have a plan to mitigate.

There's always a risk someone will get burned due to a consensus change if they aren't following along. IMO you need to just rip the bandaid off and push forward. This is an important change to the core developers that makes sense to do and with future changes down the road users that intend to interact with Counterparty will need to keep themselves as informed as possible.

I support the change as part of the continued effort by the current maintainers to both modernize Counterparty and make it more efficient and easier for users to run on their own.

@jotapea
Copy link
Contributor

jotapea commented Jul 11, 2024

My understanding of the main issues with addrindexrs are:

  1. Maintaining a separate codebase.
  2. The "original" Counterparty transaction model would have worked without it.
  3. Makes block parsing slower.

Point 1: Can be solved by replacing it with an "industry standard" alternative, like electrs. AND this has already been DONE, see https://github.com/jotapea/counterparty-core/commits/master/.

Point 2: Like @droplister mentioned "what's done is done". Meaning, that for the rest of time, the verifiability / auditing of the ledger of transactions of the protocol will require the functionality provided by this software. AND the PSBT UTXO-balances feature ALSO breaks the "original" Counterparty transaction model!

Point 3: Yep, it does. And like all software, there are tradeoffs. And this is a nuance point because of the many things that must be considered: block parsing vs. steady state (following the tip); bootstrap; hard-coded transactions; code complexity / maintainability; etc...

The multiple added limitations in a single release to the "dispensers" feature is basically like killing it, but keeping the same name for appearances. Limitations to the existing feature set should only be acceptable AFTER a better feature has been proven to make the old feature irrelevant. Or at the very least just having the alternative ready BEFORE.

Having "indexed addresses" is a natural for Counterparty which is (as of today still 100%) address based. After having proved that addrindexrs can be replaced by a vanilla electrs, I just won't accept that argument as valid any longer. Counterparty is more feature-full by including it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests