Implement compose_dispense

[ouziel-slama]

Normally it's finished but more tests are needed..
I tried to modify as little code as possible.

1. Add a compose_dispense() (using OP_RETURN with the CNTRPRTY prefix)
2. Have compose_send('BTC') (on both API v1 and v2) guess whether the TX should trigger a dispenser—if it does, silently turn it into a compose_dispense().

[jdogresorg]

NACK - (not a core dev, so have no say, but was asked to weigh in on this specific PR by @adamkrellenstein before release )

Thank you for making me aware of this "mandatory protocol change", as I appreciate being made aware of changes to the protocol before they are put into a release.

IMO this "DISPENSE" change is entirely unnecessary. While it is nice that there is an "Alternative Dispenser Upgrade Path" that does not immediately force users to some new method and immediately break the old method, I still strongly DISAGREE with the need for any change to dispensers at this time.

CP devs are able to update Counterparty and add PSBT and atomic swap support WITHOUT removal of addrindexrs and WITHOUT changing dispensers at all. I realize current core devs would like to make these dispenser changes, but they are unnecessary, have been objected to for various reasons by a number of devs, and from my perspective and numerous other devs, is being forced on the community.

I would strongly suggest that CP development efforts focus on Partial Signed Bitcoin Transactions (PSBT) and atomic swaps, and that any future mandatory "protocol change" releases that are put out have additional functionality which users are requesting. Users have to WANT to update to the latest release, and can NOT be forced to update.

As I do not feel there is community consensus behind this dispenser change, I do not have plans to update to any version of Counterparty that changes dispensers or their functionality at this time.

[adamkrellenstein]

This PR fixes critical vulnerabilities in the protocol that are entirely independent of future feature development. (These are vulnerabilities that @jdogresorg himself partially addressed with multiple mandatory protocol changes just last year.)

[0xAlik]

As far as I thought, community was pretty much agree on the changes :/

[jdogresorg]

critical vulnerabilities in the protocol

Please explain in detail these critical vulnerabilities, and why they must be addressed in this release before adding additional features? Myself and others have asked many times for the technical details and continue to get general "fixes critical vulnerabiltiies" and "this is a bug" responses, but nothing concrete addressing WHY you must make the changes NOW before adding additional functionality.

Vulnerabilities that @jdogresorg himself partially addressed with multiple mandatory protocol changes just last year.

Yes, when flaws are discovered, they were CLEARLY addressed, discussed, and fixed in a future release.... However I am not seeing details on specifically what issues are being fixed here, nor why they are necessary now.

This appears to me as more misdirection and "J-Dog did changes in the past, so we should be able to" vs addressing the real questions (which have been asked on Github, Telegram, DMs, etc for many weeks) of "What critical vulnerabilties" and "Why Now before adding functionality"

[jdogresorg]

As far as I thought, community was pretty much agree on the changes :/

Consensus is hard... as it is not a democracy, votes don't matter, and in a case of small community like CP, any contentious change could fork the chain unnecessarily and should be avoided at all costs (hence not forcing a XCP fee on numerics during 2023).

If there is any doubt about community consensus, then there IS NO CONSENSUS, and a change should not be included in a release.

Now back to lurking :)

[adamkrellenstein]

The motivation for this change has already been explained and discussed ad nauseam. This particular PR is for a fully backwards-compatible change that prepares for the resolution of #1785 and #1670, the latter of which describes in detail why the current protocol design is broken (with screenshots, graphs and charts galore).

[jdogresorg]

the latter of which describes in detail why the current protocol design is broken (with screenshots, graphs and charts galore, none of which has ever been mentioned in the expression of any opposition to the proposed fix).

So to clarify, your reasoning that dispensers need to be "fixed" NOW before you can add any additional functionality to CP like atomic swaps and PSBT are :

1. Issue Require Dispenser Address to be Source Address #1785 - You discovered that dispensers can open up on EMPTY addresses (to save on wasted BTC and asset transfer fees) and want that logic removed as a "bug"... you personal interpretation of a "feature", and created a github issue 2 weeks ago.

2. Issue Make dispense a Normal Counterparty Transaction #1670 - You feel that the paying of BTC directly to dispensers is a "bug"... your personal interpretation of a "feature", and created a github issue in April to change this feature, thereby making "dispense" a counterparty action, making the "dispense" transaction larger, cost more in tx fees, and break the ability to pay a dispenser from any BTC wallet (something CP has had for 4+ years)

I do appreciate you taking the time to link a couple github issues, however the fact one of those issues charts and such does not demonstrate to me why these changes have to be made now, BEFORE any improvements to PSBT and atomic swaps work can be done. They certainly demonstrate that parsing can be done a bit faster with these changes, but still fail to demonstrate why these changes are necessary at this time, nor justify why the faster parsing is 100% necessary at this time (blocks are every 10 minutes, CP parses blocks under a minute in most cases, plenty fast enough for now... could be made faster sure, but not absolutely necessary as it is being represented to the community)

I feel this is once again not directly answering questions, trying to guide the community into thinking this issue has been discussed at length by the community and developers and that there is consensus, when in reality many devs have been trying to get straight answers for weeks, and are frustrated by the lack of technical responses and some of the personal attacks, and there is no consensus on this change.

[adamkrellenstein]

The justification for making these changes "NOW" is that they address critical vulnerabilities that lead to the regular loss of funds by users, as evidence by the numerous warning banners displayed prominently on XChain. As far as I'm concerned, all future feature development takes a backseat to fixing known vulnerabilities. If you @jdogresorg want to fork the network (again) because you simply don't like that we're fixing these bugs sooner rather than later, that's your prerogative. AFAIK, the only "dev" that has objected is you, and you are (at your own request) no longer a Counterparty developer; you have been uninvolved in these discussions (again, at your own request).

[jdogresorg]

If you @jdogresorg want to fork the network (again)

I think you forget how forks work man... Node operators can not be forced to update, you are well aware of this fact and you are choosing to release software which will "fork" the network while trying to paint me as the bad guy. I am not the "fork", you and your protocol changes without community consensus are the fork sir.

AFAIK, the only "dev" that has objected is you

Once again, I feel this is not engaging in a genuine way, as there is a clear pattern of shouting down anyone who objects in telegram telling them to weigh in on github, then a clear pattern of ignoring the feedback on github issues.

@jotapea @davestaxcp @b1u3y @B0BSmiths as well as a handful of users in the telegram channels have raised concerns, which are minimized and ignored.

It is clear to me your going to do what You feel is best, and if ADAM considers something a bug, he is going to "fix" it regardless of what the community feels.

[jotapea]

I think we should wait until more production nodes are in v10 before any protocol change.

My product (xcp.dev) is still in v9, but I hope to have a production-ready v10 really soon (in days, not weeks). And in my experience with v10, it is slow in many operations when compared to v9. I believe these should be addressed before forcing protocol changing node updates.

---

Preliminary feedback to implementation:

The current implementation makes a tight coupling between dispense and send. Maybe, a better approach would be to just add a prefix to classic BTC send. And, I would like to know why not consider adding it as a cleartext in OP_RETURN? This will allow non-CP wallets to continue doing these with minimal adjustments... (and is free advertisement for the protocol in every block explorer).

BUT, I still believe we should continue supporting non-prefixed BTC sends (at least until alternatives make it irrelevant, shown in volume). I don't think this is a broken way, because it favors UX, specially for non-CP wallets. I like that this is possible, it welcomes any Bitcoin user to Counterparty... with risks obviously, because it bypasses any validation.

And I'm not sure what this implementation does with multi-output dispenses...

[niftyboss1]

People have lost significant amounts of money when withdrawing BTC from a CEX to an address that still had an active dispenser on it. Some people rage quit Counterparty over this. Making "dispense" a Counterparty transaction resolves this issue, so I fully support deploying a fix with high priority.

[jotapea]

@niftyboss1 I can agree that it is flawed, as that is the tradeoff of being stupid simple.

[adamkrellenstein]

I think we should wait until more production nodes are in v10 before any protocol change.

My product (xcp.dev) is still in v9, but I hope to have a production-ready v10 really soon (in days, not weeks). And in my experience with v10, it is slow in many operations when compared to v9. I believe these should be addressed before forcing protocol changing node updates.

v10 has now been out for six weeks, and practically speaking, the proposed protocol change wouldn't go into effect for six more. Three months is plenty of time to upgrade, especially given the extended alpha and beta release periods we had.

There are definitely some operations which are moderately slower with the new DB design, but every reported significant slowdown has been fixed AFAIK. If that's not the case, feel free to open an issue (listing specific API calls and a comparison between v9 and v10).

The current implementation makes a tight coupling between dispense and send. Maybe, a better approach would be to just add a prefix to classic BTC send. And, I would like to know why not consider adding it as a cleartext in OP_RETURN? This will allow non-CP wallets to continue doing these with minimal adjustments... (and is free advertisement for the protocol in every block explorer).

This not a "tight coupling"—this is just adding a prefix to a classic BTC send. Please read the actual PR before commenting. The reason that it's not in cleartext is that all Counterparty messages are encoded in the same way, and, in general, using non-Counterparty wallets to make Counterparty transactions is fundamentally unsafe (without atomic swaps). If you don't see that, despite all of the evidence presented, I can't help you. 🤷‍♀️

BUT, I still believe we should continue supporting non-prefixed BTC sends (at least until alternatives make it irrelevant, shown in volume). I don't think this is a broken way, because it favors UX, specially for non-CP wallets. I like that this is possible, it welcomes any Bitcoin user to Counterparty... with risks obviously, because it bypasses any validation.

This PR does allow for vanilla BTC sends to continue to work. :/ The whole point of this PR is to get everyone to switch in a backwards-compatible way. Please read the actual PR before commenting...

And I'm not sure what this implementation does with multi-output dispenses...

I'm not sure what you're referring to. This is a backwards-compatible change. Please read the actual PR before commenting. :/

[loon3]

For years many were under the impression (myself included) that addrindex was a necessary evil for Counterparty to parse blocks. The reality is that it was only needed (up until the much more recent empty address dispenser requirement) for Counterwallet to query UTXOs by address. Removing this large dependency is a major win for Counterparty (and eliminates any future issues addrindex might have wrt its compatibility with bitcoin updates, for example Taproot addresses) and it can be done while still keeping dispensers by updating them in this fashion.

The other benefit for allowing opening of dispensers on your own counterparty address is it becomes much easier for users to "trust" the dispenser operator as well as making it easier to manage dispenser funds.

This update will require user education as well as updates to Counterparty wallets but the fact that we can not only remove the addrindex depency but also get a few benefits in the process should not be understated.

[jotapea]

I am reading:

counterparty-core/counterparty-core/counterpartycore/lib/messages/versions/send1.py

Line 78 in f9b412b

return dispenser.compose_dispense(db, source, destination, quantity)

counterparty-core/counterparty-core/counterpartycore/lib/messages/versions/enhanced_send.py

Line 110 in f9b412b

return dispenser.compose_dispense(db, source, destination, quantity)

And I'm suggesting, that maybe a better approach would be to:

• do nothing for enhanced_send, as it already contains a prefix
• add a send-specific BTC prefix (or cleartext!) to send1

Much less code and it provides the same fundamental solution of making BTC sends a Counteparty transaction.

[davestaxcp]

This update will require user education as well as updates to Counterparty wallets but the fact that we can not only remove the addrindex depency but also get a few benefits in the process should not be understated.

Carried out by who? And how?

[DerpHerpenstein]

For long time counterparty users that have already made the mistake of sending themselves BTC from an exchange and accidentally dispensing assets, the current design is normal... But from an new users perspective, this is an unacceptable bug. I think that a dispenser design that allows for a user to open a dispenser on their own address, that can only be triggered by a tx designed to trigger that dispenser is necessary to remove the complications that come with new users trying to understand current design and this PR is the first step in making that happen.

I think the intent of the original design of making every BTC tx a potential dispense was to allow for users to bring their own wallets, and this intent was great! That said, over the past few years the wallet ecosystem has begun to mature and now, a better solution to this problem is to ensure the API can return any transaction in a PSBT formal instead of raw hex so that users can use the wallet of their choice, not just for dispensers, but for ANY counterparty transaction!

With regards to this specific PR:
I'm not intimately familiar with the codebase so correct me if i'm wrong here

 # Just send BTC?
    if asset == config.BTC:
        # try to compose a dispense instead
        return dispenser.compose_dispense(db, source, destination, quantity)

Won't this change ensure all btc sends generated with the counterparty API generate a compose_dispense, or is there a fallback in compose_dispense that will create a standard BTC tx if there is no dispenser at the destination? If there is no fallback, doesn't there be a check on the destination address?

 # Just send BTC?
    if asset == config.BTC:
        if(is_dispenser(destination))
            return dispenser.compose_dispense(db, source, destination, quantity)
        else
           return (source, [(destination, quantity)], None)

[ouziel-slama]

@DerpHerpenstein @jotapea yes there is a fallback in the compose_dispense function:

def compose_dispense(db, source, destination, quantity, no_only_btc=False):
    dispensers = ledger.get_dispensers(db, address=destination)
    if len(dispensers) == 0:
        # simple BTC send
        if no_only_btc:
            raise exceptions.ComposeError("address doesn't have any open dispenser")
        return (source, [(destination, quantity)], None)
    # sanity check
    for dispenser in dispensers:
        if dispenser["status"] != STATUS_OPEN:
            raise exceptions.ComposeError("dispenser is not open")
        if dispenser["give_remaining"] == 0:
            raise exceptions.ComposeError("dispenser is empty")
        must_give = get_must_give(db, dispenser, quantity) * dispenser["give_quantity"]
        if must_give > dispenser["give_remaining"]:
            raise exceptions.ComposeError("dispenser doesn't have enough asset to give")
    # create data
    data = struct.pack(config.SHORT_TXTYPE_FORMAT, DISPENSE_ID)
    data += b"\x00"
    return (source, [(destination, quantity)], data)

So when you make a create_send the script checks if the destination is a dispenser:

• if yes, the script performs some sanity check and insert a dispense prefix.
• if not, the script return a vanilla BTC transaction like before.

Vanilla BTC are still supported. This PR is fully backward compatible and completely transparent for user and developer.

I don't understand @jdogresorg problem. Even if you think there are other priority modifications, like atomic swap, that doesn't make this PR irrelevant. It seems to me that this is more of a personal problem with Adam than a technical problem.
Personally I agree that the priority is to fix all known issues and clean the code thoroughly before adding new features. It seems obvious that it is healthier to build on solid foundations.

[ouziel-slama]

My product (xcp.dev) is still in v9, but I hope to have a production-ready v10 really soon (in days, not weeks). And in my experience with v10, it is slow in many operations when compared to v9. I believe these should be addressed before forcing protocol changing node updates.

API v2 is more complete, easier to user, and optimized for the new database structure (NO UPDATE).
If you are not using the API, I recommend you to check this module: https://github.com/CounterpartyXCP/counterparty-core/blob/develop/counterparty-core/counterpartycore/lib/api/queries.py#L132
the only dependency is json so you can easily import it.

[Jpja]

@loon3 has a very good point.; by requiring a compose_dispense message we can get rid of addrindex. This alone is worth the change imo.

I opened a separate issue about ARC4 encoding. If we remove this requirement it will be easier to buy from general Bitcoin wallets.

I have some questions about the implementation:

Did I read @jotapea right? Will buying from dispenser fall under classic send and enhanced send messages? I agree with jotapea that a new message ID for buying from dispenser is preferable.

Also, does the message specify the specific dispenser to buy from OR keep the current functionality where all dispenser from that address is triggered? The latter is actually a useful feature (eg for card bundles) albeit a spam vector (put 1000 dispensers on one address).

[ouziel-slama]

@Jpja here what the PR is doing:

When you call a create_send with asset = BTC (classic or enhanced) the script checks if the destination is a dispenser:

• if yes, the script performs some sanity check and insert a dispense prefix (I didn't invent a prefix, one already exists in the JDog implementation, I'm just using it).
• if not, the script return a vanilla BTC transaction like before.

When you call a create_dispense the script checks if the destination is a dispenser:

• if yes, it performs some sanity check and insert a dispense prefix
• if not it raises an error.

Also, does the message specify the specific dispenser to buy from OR keep the current functionality where all dispenser from that address is triggered?

The prefix is super short (b'\r\x00') and contains no information. None of the dispenser's functionality is modified by this PR. Its goal is:

• add guardrails to prevent, where possible, users from losing funds by sending BTC to a closed/empty/non-existent dispenser.
• it will enable us to significantly accelerate the initial parsing of blocks. This is fundamentally important to encourage the onboarding of new node maintainers and third-party application developers.

[adamkrellenstein]

This update will require user education as well as updates to Counterparty wallets but the fact that we can not only remove the addrindex depency but also get a few benefits in the process should not be understated.

Carried out by who? And how?

This PR will actually not require any education. It is fully backwards-compatible. The API will be backwards-compatible. User behavior will not be affected in the slightest. :/

[ShabanShaame]

From what I understand on what was previously stated by @adamkrellenstein the PR "is fully backwards-compatible" in that case I don't see any issues merging it.
But I may be missing something @jdogresorg and others if this is an enhancement that doesn't impact current workflow would you still oppose this change ?

[jotapea]

I think most people here agree that dispensers can be improved.

But, IMO, this is not being implemented in the best way (and is too soon for a protocol changing mandatory upgrade (who here actually runs nodes?)).

If a separate create_dispense CNTRPRTY message type is desired, fine, go ahead and do it. The main issue I have with this implementation is with the send becoming a dispense message. Send should just be its own message. And I would take the opportunity to make them in cleartext (thank you @Jpja for formalizing it).

[adamkrellenstein]

@jotapea What you

I think most people here agree that dispensers can be improved.

But, IMO, this is not being implemented in the best way (and is too soon for a protocol changing mandatory upgrade (who here actually runs nodes?)).

If a separate create_dispense CNTRPRTY message type is desired, fine, go ahead and do it. The main issue I have with this implementation is with the send becoming a dispense message. Send should just be its own message. And I would take the opportunity to make them in cleartext (thank you @Jpja for formalizing it).

Do not post here if you have read and understood the PR. The PR does create its own message. That requires a protocol change. It is, however, 100% backwards-compatible. Send is still its own message. 🤦‍♀️🤦‍♀️🤦‍♀️

[jotapea]

If you are not using the API, I recommend you to check this module: https://github.com/CounterpartyXCP/counterparty-core/blob/develop/counterparty-core/counterpartycore/lib/api/queries.py#L132
the only dependency is json so you can easily import it.

Very nice @ouziel-slama, thank you for the reference (and the implementation!).

[jotapea]

Thinking that dispense is the equivalent to order_matches when considering their validity.

Users create orders, and then the system determines the matches. Thus all order_matches data is implicitly valid, because this is not a message written in the transaction.

Dispenses are also implicitly valid, because the system creates them based on enough BTC being sent to an address.

So, adding a compose_dispense with its specific transaction message would mean that dispense becomes a user entered message, which can be invalid, thus breaking protocol assumptions.

NACK to this specific implementation, #1814 seem like the better path forward.

[adamkrellenstein]

@jotapea you have already been warned about posting nonsense on GitHub multiple times. If you continue to do so you will be blocked. Not because I disagree with what you're writing, but because it makes absolutely zero sense.

[jotapea]

Even if the protocol doesn't specify 'user' vs 'system' messages, I think there is a clear distinction between them. This is the perspective I am presenting.

User entered messages can be invalid, then ignored by the protocol consumers. System messages are all valid, thus cannot be ignored. This is how I (and I assume many others) understand the events written in the messages table.

(I can be wrong, and if I am, I would appreciate being told in what specifically. To learn.)

For example, I want to buy from a dispenser that is asking for 0.0011 BTC. I send 0.001 BTC in a transaction with the proposed dispense message... but this transaction won't trigger the dispense.

How will this transaction look in the db? My understanding is that it will be inserted into the dispense table...

Which has no status field... for some "nonsensical" reason...

...

On the other hand, BTC sends with a CNTRPRTY message can be sent to a dispenser, and the dispense entry will only happen IF the dispense was triggered. Thus, continues being consistent as a system-only message that doen't need to specify validity.

[adamkrellenstein]

Per #1670 (comment), this PR will be recreated according to the new implementation strategy described here: #1825