Prepare Liquid2 merge into dev - #366
Conversation
|
Classification update: this is production-oriented Liquid integration work, not a staging hotfix, but the very broad historical diff is not ready for an upstream merge decision. It remains draft pending focused maintainer review of migrations, authorization boundaries, callback behavior, and a scope split that separates runtime fixes from the readiness harness. |
The liquid2 lineage predates the dev-side change that made the lightning port exposed by default, so merging would silently flip the v0.2.4 default back to false. Configs still at schema_version 0.2.4 get defaults injected from this schema (ajv useDefaults) before migration, which would close inbound LN connectivity on upgrade. Align with dev and with config-v0.2.5/0.2.6.
elements_newblock() was deleted when new-block handling moved to the MQTT newtip flow (blocknotify -> cyphernode/elements/newtip -> elements_node_newtip.sh), but the requesthandler dispatch case and the gatekeeper registrations were left behind, so the endpoint dispatched to a function that no longer exists. Drop the case and the action_elements_newblock entries, and flip the regression test to guard against the stale registration coming back.
The selector whitelist accepts 01-04 but createWallets.sh only provisions spending01.dat, which reads like a bug. This mirrors the Bitcoin spender convention (extra spending wallets are created by the operator), so keep the range and state the constraint where the whitelist lives.
eventMessage is caller-supplied, so a double quote or backslash in it produced malformed JSON on the elements_tx_confirmation and elements_spend MQTT topics and in the paymentalist check_bolt11_mrh request body. Build the payloads with jq -nc so values are escaped; this also collapses the duplicated assetId branches in elements_spend.
Dispatching on ${1} made the documented ./mine.sh <nbblocks> form a
silent no-op and let sourcing scripts trigger mining through their
own positional parameters (six tests source this file). Guard the
dispatch on ${0} again, keep the bitcoin/elements subcommands,
default bare arguments to bitcoin, and document the forms.
jq -r of a missing .assetId yields the literal string null, which elements_spend then stored in elements_recipient.assetid as a 4-character varchar and echoed back as "assetId":"null". Store SQL NULL and return JSON null when no asset was specified.
The executecallbacks cron path ran elements_manage_not_imported, elements_manage_missed_conf and elements_do_callbacks on every tick even on Bitcoin-only nodes. Gate them on FEATURE_ELEMENTS like startproxy.sh gates the Elements notification listeners; requesthandler.sh already sources db/config.sh.
Bitcoin getaddressinfo is watcher-gated even though it is served by the spender node, and the other Elements read endpoints (validateaddress, gettransaction) are watcher too. The gatekeeper level expresses caller privilege, not node routing, so align the Elements endpoint with the convention.
None of its seven functions is dispatched by any requesthandler case or registered gatekeeper action, and the dead code carries latent bugs (a query on a nonexistent watching_id column, raw SQL string splices). Remove it from the merge; wiring the Elements active-watch endpoints up properly can come as its own PR reusing the getactivewatches.sh patterns.
| "type": "boolean", | ||
| "title": "Expose lightning node", | ||
| "default": true, | ||
| "default": false, |
There was a problem hiding this comment.
[review] lightning_expose default flipped to false in the v0.2.4 schema
dev has default: true here, and config-v0.2.5/0.2.6 in this branch still say true — this false comes from the branch's pre-2020 lineage, so merging would regress dev. Configs still at schema_version 0.2.4 get defaults injected from this schema (ajv useDefaults) before migration, so an upgrade would silently drop the 9735:9735 port mapping and close inbound LN connectivity.
Fixed in f495017.
| elements_newblock) | ||
| # curl (GET) 192.168.111.152:8080/elements_newblock/c7168456b69248e166a0a39ed61dbb19623de0d65789a20d5b39bc6b4371dfbb | ||
|
|
||
| response=$(elements_newblock "${line}") |
There was a problem hiding this comment.
[review] elements_newblock dispatches to a function that no longer exists
elements_newblock() was deleted in 57b69ad when new-block handling moved to the MQTT flow (blocknotify → cyphernode/elements/newtip → elements_node_newtip.sh), but this dispatch case and the action_elements_newblock=internal gatekeeper registrations were left behind — any call exits 127 with an empty response.
Fixed in a5775e2 by removing the dead endpoint (and flipping the regression test that asserted the stale registration existed).
|
|
||
| validate_elements_spender_wallet() { | ||
| case "${1}" in | ||
| 01|02|03|04) return 0 ;; |
There was a problem hiding this comment.
[review] selectors 02–04 validate but only spending01.dat is ever created
createWallets.sh only provisions spending01.dat, so a request with "wallet":"02" passes validation and then fails in elementsd with "Requested wallet does not exist or is not loaded". This mirrors the Bitcoin spender convention (extra spending wallets are operator-created), but nothing said so.
Documented in 8130cde — kept the 01–04 whitelist, added a comment stating 02–04 are reserved for operator-created wallets.
| # Publish the selected output if requested. | ||
| if [ -n "${event_message}" ]; then | ||
| trace "[elements_confirmation] Publishing elements_tx_confirmation for tx ${txid}, watch ${watching_id}, vout ${tx_vout_n}" | ||
| response=$(mosquitto_pub -h broker -t elements_tx_confirmation -m "{\"txid\":\"${txid}\",\"hash\":\"${tx_hash}\",\"address\":\"${address}\",\"unblindedAddress\":\"${unblinded_address}\",\"vout_n\":${tx_vout_n},\"amount\":${tx_vout_amount},\"watchingAssetId\":\"${watching_assetid}\",\"assetId\":\"${tx_vout_assetid}\",\"confirmations\":${tx_nb_conf},\"eventMessage\":\"${event_message}\"}") |
There was a problem hiding this comment.
[review] MQTT payload built by raw string-splicing caller-supplied text
eventMessage is caller-supplied at watch time; any double quote or backslash in it produced malformed JSON on the elements_tx_confirmation topic, so subscribers would fail to parse the confirmation event. Same pattern in elements_walletoperations.sh (elements_spend event) and paymentalist.sh (check_bolt11_mrh body).
Fixed in 196989a — all three payloads are now built with jq -nc, which also collapsed the duplicated assetId branches in elements_spend.
| } | ||
|
|
||
| case "${0}" in *mine.sh) mine "$@";; esac | ||
| case "${1}" in |
There was a problem hiding this comment.
[review] documented ./mine.sh <nbblocks> became a silent no-op
Dispatching on ${1} instead of ${0} broke the documented direct invocation (exits 0 without mining — confirmation tests just hang) and made sourcing side-effectful: the six test scripts that source this file would trigger mining if their own first argument were bitcoin/elements.
Fixed in 57721f5 — dispatch guarded on ${0} again, bitcoin/elements subcommands kept, bare args default to bitcoin.
| " RETURNING id" \ | ||
| "SELECT id FROM elements_tx WHERE txid='${txid}'") | ||
| trace_rc $? | ||
| sql "INSERT INTO elements_recipient (address, unblinded_address, amount, elements_tx_id, assetid) VALUES ('${address}', '${unblinded_address}', ${amount}, ${id_inserted}, '${assetid}')"\ |
There was a problem hiding this comment.
[review] omitted assetId persisted and returned as the string "null"
jq -r of a missing .assetId yields the literal string null, which was INSERTed into elements_recipient.assetid as a 4-char varchar and echoed back as "assetId":"null" for every L-BTC spend without an explicit asset.
Fixed in b2080dc — stores SQL NULL and returns JSON null.
| executecallbacks) | ||
| # curl (GET) http://192.168.111.152:8080/executecallbacks | ||
|
|
||
| response=$(elements_manage_not_imported) |
There was a problem hiding this comment.
[review] Elements maintenance runs unconditionally on Bitcoin-only nodes
executecallbacks ran elements_manage_not_imported / elements_manage_missed_conf / elements_do_callbacks on every cron tick with no FEATURE_ELEMENTS guard, unlike startproxy.sh which gates the Elements listeners.
Fixed in 4f3098a — gated on FEATURE_ELEMENTS (requesthandler already sources db/config.sh).
| trace "[elements_bumpfee] response=${response}" | ||
|
|
||
| if [ "${returncode}" -eq 0 ]; then | ||
| trace "[elements_bumpfee] error!" |
There was a problem hiding this comment.
[review] success/error log branches inverted in elements_bumpfee
Logs error! when returncode is 0 and success! when it is non-zero — anyone debugging a stuck fee-bump from proxy logs is sent in exactly the wrong direction.
Fixed in ba7ff94.
| action_bitcoin_getfeerate=watcher | ||
| action_validateaddress=watcher | ||
| action_elements_validateaddress=watcher | ||
| action_elements_getaddressinfo=spender |
There was a problem hiding this comment.
[review] elements_getaddressinfo gated at spender level, peers are watcher
Bitcoin getaddressinfo is watcher-gated even though it is also served by the spender node, and the other Elements read endpoints (validateaddress, gettransaction) are watcher too — the gatekeeper level expresses caller privilege, not node routing, so a watcher key unexpectedly got 403 here.
Fixed in 79c826c — changed to watcher in both properties files.
| return ${returncode} | ||
| } | ||
|
|
||
| elements_getactivewatches() { |
There was a problem hiding this comment.
[review] entire file is unreachable dead code with latent bugs
None of the seven functions here is dispatched by any requesthandler case or registered gatekeeper action, and the code carries latent bugs (line 44 queries a nonexistent watching_id column; raw SQL string splices at lines 15/42/136) that would bite whoever wires it up later.
Removed in 03707b6 — recoverable from history; wiring the Elements active-watch endpoints up properly deserves its own PR reusing the getactivewatches.sh patterns.
Code review — 10 findings, all fixed in b99a51f..03707b6A high-effort review of this PR surfaced 10 verified defects, each now posted as an inline comment at the flagged line (they show as outdated because the fixes are already pushed, one commit per finding): Merge blockers (were):
Behavioral/hygiene: Three additional suspicions were checked and refuted: the walletnotify guard widening, the unconditional Elements curl-config creation, and the watch-assets index migration — all fine as-is.
|
Merge-readiness harness results (post-fixes, 03707b6)Ran the
Still outstanding before merge:
|
7 participants
Summary
Prepare
features/liquid2for merge intodev, includingfeatures/elgetaddrinfo.This branch brings the Liquid/Elements integration forward onto current
dev, keeps Bitcoin behavior compatible, hardens the Liquid request/runtime paths, and adds repeatable merge-readiness test coverage.Merge strategy
The strategy was compatibility-first:
dev.getaddressinfoendpoint because the Liquid watch flow benefits from a first-class way to inspect owned Elements addresses.Commit-level rationale
elements_getaddressinfo endpointRationale:
features/elgetaddrinfo.Strategy:
watcherlevel, matching the address-inspection use case.Make Liquid integration merge-readyRationale:
dev.Strategy:
watching_assetid, matching the fresh-install schema and allowing the same address/callback pair to be watched for different assets.vout, amount, and asset come from the selected watched output.Harden Elements RPC request handlingRationale:
Strategy:
jq --argand--argjsonso request values cannot alter the JSON-RPC method or structure.Harden Liquid watch and Paymentalist runtimeRationale:
Strategy:
Add Liquid2 merge-readiness test harnessRationale:
devschema to the candidate Liquid2 schema.Strategy:
contrib/liquid2-merge-readiness/local-static.shfor local static checks, Elements shell regressions, Paymentalist Rust checks, and Docker image builds.contrib/liquid2-merge-readiness/migration-dev-to-liquid2.shto create an isolated current-devdatabase, run candidate migrations twice, and verify Bitcoin data survives while Liquid tables/indexes are present.contrib/liquid2-merge-readiness/staging-server-live-smoke.shfor live deployed-stack smoke coverage against Bitcoin Core and Elements without broadcasting spends.contrib/liquid2-merge-readiness/gatekeeper-auth-matrix.shto test every deployed Gatekeeper action against no token, invalid token, and all configured API ids.Validation
Passed:
contrib/liquid2-merge-readiness/local-static.shcontrib/liquid2-merge-readiness/migration-dev-to-liquid2.shcontrib/liquid2-merge-readiness/gatekeeper-auth-matrix.shcontrib/liquid2-merge-readiness/staging-server-live-smoke.shproxy_docker/app/tests/test-watches.shproxy_docker/app/tests/test-elements-manage-missed.shproxy_docker/app/tests/test-manage-missed.shLive staging validation covered:
Notes for reviewers