Repository navigation
fix(ticket): reject duplicated ticket ids in one tclose - #1335
Conversation
TicketClose validates and builds its tickets slice in one loop, then pays out
in a second loop that never sees the first loop's effects (the validation loop
writes nothing to the DB). A ticketId repeated in the payload is therefore read
N times as the same unmodified ticket and paid N times, while only one close is
recorded. The payout comes from the (ReturnAddress, ticket exec addr) frozen
pool -- shared by every open ticket of that address:
- the withdraw time lock is checked per ticket but the pool is per address, so
one matured ticket repeated drains the deposits of the address's
not-yet-matured tickets (TestTicketCloseDupBypassesWithdrawLock);
- closing a mined ticket also unfreezes a CoinDevFund from the chain-wide dev
fund pool on every iteration, and that pool is shared by all holders -- once
drained, other holders' tclose fails with ErrNoBalance and their tickets stay
stuck with the deposit and mined reward unwithdrawable
(TestTicketCloseDupFundDrainBlocksOtherHolders);
- the address's remaining tickets cannot be closed until mining rewards refill
the pool (TestTicketCloseDupDrainsSharedPool).
The payout always credits the ticket's own ReturnAddress, so an attacker cannot
redirect funds to themselves: a bound miner receives nothing, and the amount is
capped by the pool (TestTicketCloseDupPaysOnlyReturnAddress,
TestTicketCloseDupExtractionCappedByPool). TicketClose unfreezes only; it never
issues coins.
Reject a repeated ticketId in the validation loop, before any payout, behind the
new ticket fork ForkTicketCloseDup. Pre-fork behaviour is unchanged so historical
blocks replay as before. The fork ships as 0 in every config that carries a
[fork.sub.ticket] section, since chain33's initForkConfig panics at startup for a
registered fork with no config entry; TestTicketForkConfigDeclared guards that.
Co-authored-by: Claude <[email protected]>
devFund is already int64, so int64((fundPool-repeats)*devFund) and int64(repeats*devFund) are no-ops. golangci-lint's unconvert (enabled in golinter.sh) rejected them and failed the check_fmt job. Co-authored-by: Claude <[email protected]>
master 已合入 #1333(fix(ticket): bind the mining tx to the ticket execer in CheckBlock), 它在同一批 [fork.sub.ticket] 配置与 InitFork 注册处新增了 ForkMinerExecerCheck, 与本 PR 新增的 ForkTicketCloseDup 冲突。 冲突解决: 两个分叉都保留(缺任一条目 chain33 initForkConfig 会直接 panic): - 6 处含 [fork.sub.ticket] 的配置同时保留 ForkTicketCloseDup 与 ForkMinerExecerCheck; - types.InitFork 同时注册这两个分叉; 其余改动(master 的 CheckBlock 校验、本 PR 的 TicketClose 去重与 ErrDupTicketID)自动合并后语义不变。 回归: plugin/dapp/ticket/... 与 plugin/consensus/ticket/... 全绿。
libangzhu
left a comment
There was a problem hiding this comment.
核心修复没有发现正确性问题:重复 ticketId 会在付款循环里被重复解冻,去重放在付款之前,位置正确。plugin 配置里的 ForkTicketCloseDup=0 不算主网兼容问题,比特元主网的生效高度会在后续升级依赖时由 bityuan/bityuan.go 单独配置,历史区块仍按旧规则重放。
建议修正文档和测试说明:
- 开发基金池重复解冻只存在于
ForkTicketFundAddrV1生效之前。比特元主网该分叉高度是4320000,早已生效,TicketClose不会再解冻旧基金池。相关测试可以保留,但请标明是历史协议路径。 - 请补充当前主网的影响:重复关闭票 A 会把同一
ReturnAddress共享的冻结资金解冻到 ticket 执行器子账户的可用余额,而票 B 仍是TicketOpened。钱包选票、共识选票和TicketMiner都不检查冻结余额是否还覆盖票价,所以 B 仍能参与挖矿,解冻的余额也可以转出。“无增发”只能说明TicketClose本身不增发、钱不会转给绑定矿工,不能说明后果仅限于资金提前解冻。 TestTicketCloseDupFundDrainBlocksOtherHolders没有覆盖描述里的“补回后恢复”,也没有断言受害者账户。它直接调用driver.Exec,不经过执行器的回滚流程,证明不了失败交易的状态回滚。线上由ForkExecRollback保证:失败交易照常进块、扣手续费,状态改动全部回滚。TestTicketForkConfigDeclared建议检查 key 确实位于[fork.sub.ticket]段内,并且不要扫描 gitignore 下的本地生成配置。
去重实现、分叉门控、正常单票和多票关闭的测试没有发现问题。
…ig check 复核意见 1-4 的整改(libangzhu / bysomeone)。 docs(security): 修正影响面与可达性 - §4.3 明确开发基金池路径被系统分叉 ForkTicketFundAddrV1 门控:主链 (bityuan/bityuan bityuan.go:354 = 4320000,当前高度约 4788 万)不可达, 仅平行链(-1)与执行器 testdata(-1)可达;补按链取值表 - §4.6 拆成「主链同地址危害」与「分叉未生效链上的跨地址危害」两张表,删去 「本漏洞唯一跨地址伤害无关第三方」这一主链上不成立的结论;补复核意见 2 的 后果(选票/共识/TicketMiner 不校验冻结余额、解冻余额可转出、治理票权受影响) - 新增 §4.7 可达性与方法论(四条成因)、§9 修复必要性评估(建议分叉高度改取 大于链尖的值)、§6 修正 initForkConfig 因果倒置并补部署侧注意事项 test(ticket): 用例与断言 - 两个基金池用例更名 ...PreFork,并运行时断言 fork 未生效(requireLegacyDevFundPath) - 新增 TestTicketCloseMinedPostFundAddrV1KeepsFundPool 固定主链状态 - ...FundDrainBlocksOtherHoldersPreFork 补受害者账户断言、显式断言 driver 级 中间态(无回滚),并在新环境覆盖「基金池回填后恢复可关闭」 - TestTicketForkConfigDeclared 改为:条目必须在 [fork.sub.ticket] 段内、值必须为 0 ( -1 会被转成 MaxHeight 而永不生效)、仅扫描 git 跟踪配置、并用本包实际加载的 配置断言 IsDappFork(0,...) 为真 变异验证(4 项,均按预期失败后回滚):值改 -1 / 条目移出段 / testdata 打开 ForkTicketFundAddrV1 / ticketdb.go 短路 !IsFork 门控。 回归:go test -race ./plugin/dapp/ticket/... 全绿。
bysomeone
left a comment
There was a problem hiding this comment.
补几点说明,都不要求本 PR 改动。
1. TestTicketForkConfigDeclared 只断言键存在,不验值是否让修复生效。 它用 assert.Contains(text, pty.ForkTicketCloseDup) 匹配键名;把任一配置里该键的值改成 -1(经 -1 → MaxHeight 转换为关闭)后 10 个用例仍全绿——仓库里也没有别处加载这些 toml 并校验值。建议解析出值后再断言 cfg.IsDappFork(<链尖高度>, ty.TicketX, ty.ForkTicketCloseDup),否则「配了但没生效」也会全绿过 CI。
2. 分析文档一句因果写反(docs/security/ticket-close-duplicate-ticketid-double-payout-analysis.md:233):「testdata/chain33.cfg.toml 虽设 DisableForkCheck=true,仍补上条目以免落回 MaxHeight」。实际不会落回 MaxHeight——initForkConfig 只覆盖 toml 里写了的键,配置缺该键时保留的是 RegisterDappFork 的注册默认值 0(即从创世生效)。补条目本身与仓库惯例一致,只是这句理由会让人以为「是配置条目在保证生效」,复制模板时省掉它反而变成静默生效。
3. 影响面的一处更正。 影响表里「消耗全链共享的开发基金池(GetFundAddr() / CoinDevFund)」这条在主链上不成立:该解冻被 !chain33Cfg.IsFork(action.height, "ForkTicketFundAddrV1") 门控,而主链上这个 fork 早已生效(部署仓取 4320000,远在当前高度之前),已挖矿票关闭时不再解冻基金池。它在平行链上仍然活着(chain33.para.toml 里是 -1),本 PR 的 TestTicketCloseDupDrainsDevFundPool 用的也正是 ForkTicketFundAddrV1=-1 的 testdata,即实测的是那条配置而非主链。主链上真实可达的第三方伤害是「绑定矿工对票主的票重复 tclose,抽干票主共享押金池 → 该地址其余票暂时关不掉」(钱进票主自己账户,攻击者 0 收益)。
部署 / 上线注意(note)
ForkTicketCloseDup 配 0 的行为含义是对历史同样生效:IsFork 判的是 height >= forkHeight,所以 0 表示从创世高度起就按新规则重放。若某条链的历史里存在同一笔 tclose 内含重复 ticketId 的交易,从创世 / 全量重放会卡在该区块(该 tx 在新规则下成为失败交易,状态根与历史不一致)。高度取多少由部署侧按链的实际情况决定,此处仅作行为说明。
…e reference - §9 决策记录:维护者选择保持 0(立即生效),接受历史重放风险 - §4.6 治理票权引用改为 boardaction.go:629(voteAccount.Frozen 所在行)
|
感谢复核。四点都接受,已整改并推送( 1. 开发基金池路径只在 复查了取值:部署仓 改动:分析文档 §4.3 改为「仅限 fork 未生效的链」并补按链取值表;两个用例更名 2. 现网影响面 —— 接受,已补充 §4.6 拆成「主链(同地址)」与「fork 未生效链(跨地址)」两张表,删掉"唯一跨地址伤害无关第三方"这一在主链不成立的结论。你指出的那条已逐行确认:钱包选票( 3. 测试覆盖面 —— 接受,原用例确实证明不了回滚 现在用例显式断言 driver 级中间态(失败交易留下 4. 条目必须位于 部署侧提醒: 另:文档 §9 重新评估了"既然主链不伤害第三方,还要不要修"——结论是要修(提现时间锁被绕过、共享押金不变量被打破、回执/索引不一致,修复成本极低),但曾建议分叉高度不取 0:重复 id 只能手工构造(钱包的 id 来自索引列表,RPC/CLI 不接受外部 ticketId),取 0 换来的额外保护只是"创世至今的历史"区间,却把"历史可原样重放"押在一个无法在仓库内证伪的假设上。维护者已决定保持 0,该决策与理由已记入文档 §9;若你们认为应改为大于当前链尖的高度,我们可以调整配置、测试与文档。 |
|
三点都收到,前两点已改并推送( 1. 2. 文档 §6 的因果已改写。 3. 影响面更正已并入 §4.3 / §4.6。 主链上"消耗全链共享开发基金池"不成立:部署仓 |
make linter(CI check_fmt) 在 ticketclose_dup_test.go:719 报 unnecessary conversion: fundPool*devFund 已是 int64,外层 int64() 冗余。仅此一处,gofmt -s 与用例均通过。
## [1.76.3](v1.76.2...v1.76.3) (2026-10-19) ### Bug Fixes * **ticket:** reject duplicated ticket ids in one tclose ([#1335](#1335)) ([1458fe2](1458fe2))
|
🎉 This PR is included in version 1.76.3 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
问题
TicketClose先用一个循环校验并构造tickets切片,再用第二个循环逐个付款——而校验循环不写库。因此 payload 里重复出现的ticketId会被读 N 次为同一份未变更的票状态,同一张票被付 N 次,而库里只记一次关闭。付款来源是
(ReturnAddress, ticket 执行地址)下的冻结池,该池由这个地址所有未关闭票共享。影响
不能盗走他人资产、无增发,主链也没有跨地址伤害:入账方恒为票自身的
ReturnAddress,绑定矿工分文未得,提取额受池余额封顶(实测用例 + 变异验证)。主链上真实可达的是同一个返还地址内部的影响:ErrNoBalance,直到挖矿奖励回填该池(可恢复,非永久)TicketMiner都不检查冻结余额是否仍覆盖票价,票 B 仍是TicketOpened/TicketMined就能继续挖矿;解冻出的可用余额可由该地址私钥经 coinsWithdraw转出(复核意见 2,已逐条确认)autonomy.getAddressVotes按Frozen / (TicketPrice × precision)计票TyLogCloseTicket指向同一张票,而 Mavl 只落一次关闭「消耗全链共享开发基金池、卡住其他持票人」这条在主链上不成立(复核修正):该解冻被系统分叉
ForkTicketFundAddrV1门控(plugin/dapp/ticket/executor/ticketdb.go:451),而主链早已生效——部署仓bityuan/bityuanbityuan.go:354=4320000,当前主链高度约 4788 万。它只活在 fork 未生效的链上(平行链chain33.para.toml:331为-1,且平行链[fork.sub.ticket] Enable=0、票合约从创世即可用)。本 PR 原有两个基金池用例用的正是-1的 testdata,现已改名...PreFork并在运行时断言 fork 未生效,标明它们测的是历史协议路径;主链状态另由TestTicketCloseMinedPostFundAddrV1KeepsFundPool固定。修复
在校验循环里对
ticketId去重,在任何付款之前拒绝(ty.ErrDupTicketID),门控于新分叉ticket.ForkTicketCloseDup;分叉前行为不变,历史区块可原样重放。失败即整笔回滚,余额/冻结池/票状态均不变。分叉在所有含
[fork.sub.ticket]的配置中置0(chain33initForkConfig对缺失条目会 panic)。测试
TestTicketCloseDupTicketIDPreForkTestTicketCloseDupTicketIDPostForkTestTicketCloseNormalPostForkTestTicketCloseDupPaysOnlyReturnAddressTestTicketCloseDupExtractionCappedByPoolErrNoBalance失败TestTicketCloseDupBypassesWithdrawLockErrTime),重复到期票可解冻整池TestTicketCloseMinedPostFundAddrV1KeepsFundPoolTestTicketCloseDupDrainsDevFundPoolPreForkTestTicketCloseDupFundDrainBlocksOtherHoldersPreForkTestTicketCloseDupDrainsSharedPoolTestTicketForkConfigDeclared[fork.sub.ticket]配置:条目必须在段内、值必须为 0(-1会被转成MaxHeight、永不生效)、仅扫 git 跟踪配置,并用本包实际加载的配置断言IsDappFork(0,…)变异验证(均按预期失败后回滚):短路
ticketdb.go守卫 → 分叉后用例expected 0 / actual 200;chain33.para.toml的值改-1、条目移出段 → 配置用例失败;testdata 打开ForkTicketFundAddrV1→ 两个...PreFork用例失败;ticketdb.go短路!IsFork门控 → 主链可达性用例失败。覆盖面边界:
...FundDrainBlocksOtherHoldersPreFork只到执行器层(DriverBase.Exec不回滚,用例已显式断言该 driver 级中间态);链上回滚由区块层ForkExecRollback保证,本 PR 没有区块层端到端用例。回归:
go test -race ./plugin/dapp/ticket/...全绿。部署注意
0(维护者决定,立即生效),接受历史重放风险:若某条链历史里存在同一笔 tclose 含重复ticketId的交易,从创世/全量重放会卡在该区块。重复 id 只能手工构造(钱包的 id 来自索引列表,RPC/CLI 不接受外部 ticketId),诚实历史出现的概率极低,但无法在仓库内证明为 0。文档 §9 记录了该取舍(含"改取大于链尖高度"的备选与理由)。bityuan/bityuan当前[fork.sub.ticket]没有ForkTicketCloseDup,不同步补上会导致节点在initForkConfigpanic。docs/security/ticket-close-duplicate-ticketid-double-payout-analysis.md(§4.3/§4.7 可达性与方法论、§9 修复必要性评估)。🤖 Generated with Claude Code