Silent payment index (for light wallets and consistency check)

Sjors commented at 9:13 am on August 9, 2023: member

This PR adds an index with the silent payment tweak for every transaction. It builds on top of #28122. Supersedes #24897.

Usage: bitcoind -bip352index

The index does not have wallet dependencies, so it can be built with --disable-wallet.

It also adds a getsilentpaymentblockdata RPC that returns an array of silent payment tweaked public keys, one for each qualifying transaction.

This index serves two purposes:

Light client support, see BIP 352
A more thorough check than the test vectors; e.g. by comparing a checksum of the index for all of mainnet.

TODO:

check correctness against another implementation
(maybe) implement cut-through (-bip352ctindex)

DrahtBot commented at 9:13 am on August 9, 2023: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process. A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#29491 ([DO NOT MERGE] Schnorr batch verification for blocks by fjahr)
#29483 (test, ci: add –v1transport option, add –v2transport to a CI task by mzumsande)
#29432 (Stratum v2 Template Provider (take 3) by Sjors)
#28955 (index: block filters sync, reduce disk read operations by caching last header by furszy)
#28710 (Remove the legacy wallet and BDB dependency by achow101)
#28122 (Silent Payments: Implement BIP352 by josibake)
#27351 (wallet: add seeds argument to importdescriptors by apoelstra)
#27260 (Enhanced error messages for invalid network prefix during address parsing. by russeree)
#24539 (Add a “tx output spender” index by sstone)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

Sjors commented at 9:15 am on August 9, 2023: member

I’d like to keep this based on #28122 but that requires some refactoring there. See #28122 (comment).

josibake commented at 9:39 am on August 9, 2023: member

I’d like to keep this based on #28122 but that requires some refactoring there. See #28122 (comment).

you can also rebase on #27827 , which has everything, or just the receiving PR. Might be easier then to reason about which functions belong in which PRs if we can see it all together. I think the functions you need are introduced in the receiving PR (#28202)

DrahtBot added the label CI failed on Aug 9, 2023

Sjors commented at 11:40 am on August 9, 2023: member

I’ll take a stab at rebasing on the bigger thing. It makes sense to do that and then unrebase / debase(?) it later.

Sjors force-pushed on Aug 9, 2023

Sjors commented at 2:57 pm on August 9, 2023: member

I ended up “borrowing” some code from the followup PRs, without having to rebase on them.

This code compiles and the index doesn’t crash. That’s about the extend of it though :-) For now.

Sjors force-pushed on Aug 9, 2023

Sjors force-pushed on Aug 29, 2023

Sosthene00 commented at 12:23 pm on September 4, 2023: none

Hi, I have a little question about the motivation for making this index in Core. Afaiu, an implementation of electrum server (or any light client backend) could construct this index as easily, and I think it would be better separation of concerns to let them take responsibility for this instead of Core. Or maybe it’s necessary for the consistency check you’re mentioning? I’m not sure of what it is.

Sjors commented at 2:39 pm on September 4, 2023: member

@Sosthene00 I find it useful to have this index for developing and reviewing the silent payment implementation. But as you say, it may not be worth merging eventually. I’m not sure about that yet.

There’s also the possibility of building something like this on top of the Kernel, when that project is a bit further along.

DrahtBot added the label Needs rebase on Sep 7, 2023

Sjors force-pushed on Sep 19, 2023

Sjors commented at 2:49 pm on September 19, 2023: member

Rebased to use GetSilentPaymentsTweakDataFromTxInputs. Otherwise still WIP.

DrahtBot removed the label Needs rebase on Sep 19, 2023

Sjors commented at 4:20 pm on September 19, 2023: member

Pushed a simple RPC method to get the tweaked public key for a given transaction. I’d like to sanity check it against some other implementation.

Signet: dacb55db5c506a34771bf8ac3664e2510c17e80ada81347300fb4223f2d6c545

getsilentpaymenttxdata result: 03fc95cfeac43e9179c002d32a042626e5a2b289a8d7a4a0c88fbf81d570cef987

DrahtBot added the label Needs rebase on Sep 19, 2023

Sjors force-pushed on Sep 20, 2023

DrahtBot removed the label Needs rebase on Sep 20, 2023

in src/index/silentpaymentindex.cpp:64 in 0235ff2891 outdated

59+        TxoutType whichType = Solver(vout.scriptPubKey, solutions);
60+
61+        if (whichType != TxoutType::WITNESS_V1_TAPROOT) {
62+            return false;
63+        }
64+    }

theStack commented at 3:03 pm on September 20, 2023:

I think this logic doesn’t match the BIP, as it currently implements “all outputs have to be taproot outputs” rather than “at least one output has to be a taproot output”? Could use std::none_of to achieve the latter (https://en.cppreference.com/w/cpp/algorithm/all_any_none_of).

Something like

0    if (std::none_of(tx.vout.begin(), tx.vout.end(), [](const TxOut& txout) {
1            std::vector<std::vector<unsigned char>> solutions;
2            return Solver(vout.scriptPubKey, solutions) == TxoutType::WITNESS_V1_TAPROOT;
3        }) {
4        return false;
5    }

(untested)

Sjors commented at 4:09 pm on September 20, 2023:

@josibake it sounds like this should be another helper function. The index shouldn’t be too far in the weeds of the BIP rules.

However, I’ll look into fixing this.

Sjors commented at 4:26 pm on September 20, 2023:

Should be fixed now.

Sjors commented at 5:42 pm on September 20, 2023:

Oops, pushing new fix.

Sjors force-pushed on Sep 20, 2023

Sjors commented at 4:13 pm on September 20, 2023: member

@fjahr since you’ve been working on indexes, do you have any feedback on indexer best practices I should follow?

Sjors force-pushed on Sep 20, 2023

in src/index/silentpaymentindex.cpp:103 in 91fd02f447 outdated

100+
101+    assert(block.data);
102+
103+    Consensus::Params consensus = Params().GetConsensus();
104+
105+    if (block.height < consensus.vDeployments[Consensus::DEPLOYMENT_TAPROOT].min_activation_height) {

Sjors commented at 4:30 pm on September 20, 2023:

It might be nice if the indexer base class can skip ahead to a minimum height, as it takes a bunch of time to go through all the blocks.

fjahr commented at 9:09 pm on September 28, 2023:

Yeah, we don’t have the possibility to skip blocks yet, so I think we would need to add it as a feature on base index, I have started drafting it here, though it’s not functional yet: https://github.com/fjahr/bitcoin/commit/cd7bf1249765a011210653c393b3a0fdb7106d55

But I also guess 2h doesn’t sound extremely bad, we might want more than one silentpaymentindex to motivate something like this.

Sjors force-pushed on Sep 20, 2023

Sjors commented at 9:42 am on September 21, 2023: member

The index (as generated by 46b32b63d57a62466a2b2181e28bab477dc8de95) is 54GB on mainnet and took about 2 hours to generate (of which much time was wasted looping through pre-taproot blocks). Something like #26966 could make it even faster.

I’d like to compare the results in this mainnet index to another implementation, if someone can point me to one.

Sjors force-pushed on Sep 21, 2023

Sjors commented at 8:34 am on September 28, 2023: member

@setavenger you mentioned using a light client in the tracking issue. Does that have an index or some other way to compare to this PR?

setavenger commented at 11:15 am on September 28, 2023: none

@setavenger you mentioned using a light client in the tracking issue. Does that have an index or some other way to compare to this PR?

Hmm I don’t think it matches exactly what you are doing here. My backend was built as a “light client” itself and with the assumption that it would only index forward looking from a certain activation point. It does create an index but it’s not running a full node which means there’s a lot of networking happening to services that offer prevout information per transacton. I would be surprised if it would take less than two hours to index the entire chain. Plus I would probably get rate limited before even being able to index the entire chain.

That being said I was thinking of upgrading my backend to using a fullnode I could give an update then.

Sjors commented at 1:33 pm on September 28, 2023: member

I see. Even a few spot checks against what this index produces would be useful though.

setavenger commented at 3:15 pm on September 28, 2023: none

Yeah, we could do that perhaps for selected blocks. I just have to get the handling of P2PKH inputs in order, then I should be able to index any block from the chain.

in src/init.cpp:958 in 432b7a235a outdated

916@@ -905,6 +917,8 @@ bool AppInitParameterInteraction(const ArgsManager& args)
917     if (args.GetIntArg("-prune", 0)) {
918         if (args.GetBoolArg("-txindex", DEFAULT_TXINDEX))
919             return InitError(_("Prune mode is incompatible with -txindex."));
920+        if (args.GetBoolArg("-silentpaymentindex", DEFAULT_SILENT_PAYMENT_INDEX))
921+            return InitError(_("Prune mode is incompatible with -txospenderindex."));

fjahr commented at 5:54 pm on September 28, 2023:

silentpaymentindex?

Sjors commented at 7:54 am on September 29, 2023:

I should probably get rid of this error, since the entry for each block is independent, pruning shouldn’t be a problem.

fjahr commented at 7:54 pm on September 28, 2023: contributor

Just a very rough first pass but I think you are going part of the way to treat this similar to the blockfiltertypes/blockfilterindexes but not fully, e.g. using “0” as the default but then not having an accompanying SilentPaymentType enum and something like g_enabled_filter_types in init. I think that is also why I currently have to set -silentpaymentindex=0 explicitly if I don’t want to activate it in this branch.

I am not far enough down the Silent Payments rabbit hole to judge conceptually if we need different types but for blockfilterindexes there also was initially the idea that there would be many different types and that hasn’t really played out in reality so far. So that would be question to discuss how likely it really is that users will use more than one index. Unless we are really sure users will want different ones we should probably keep it simple. So I would recommend also for this PR to just focus on having one type/index.

Of course when we consider this more seriously it will need reorg robustness and tests.

Sjors commented at 8:03 am on September 29, 2023: member

Silent Payments is built with versioning in mind. They’re not types like with BIP157 filters, but rather ways to support transactions that have inputs with new script patterns.

Unless there’s no more soft forks, it’s quite likely to actually need these new versions. E.g. any new taproot version will need a new silent payment version in order to be able to spend from it.

That said, I have not thought at all on how to handle such versioning. Although the -silentpaymentindex takes a version number, the index itself doesn’t have versioning. I’ll have to study the BIP more closely to figure out how versioning works.

The most simple solution is to just redesign and rebuild the index by the time that’s needed. But if basic versioning can be supported with little code complexity and storage overhead, I would prefer that.

Added a note to the PR description to address this.

I currently have to set -silentpaymentindex=0 explicitly if I don’t want to activate it in this branch

Will look into that along with the above. The index should be off by default. I probably want the user to set the maximum supported silent paymnets version. That way they won’t automatically upgrade later (which might involve increased storage requirements). But since the first version is 0 that’s an ambiguous argument. Adding a v prefix would be an easy solution:

-silentpaymentindex=0: index is off (default)
-silentpaymentindex=v0: index version 0

In the future, either:

-silentpaymentindex=v1: index version 0 and 1; or
-silentpaymentindex=v1: index only version 1 (makes no sense if the v1 index is a superset of v0, so I have to read up on this)

it will need reorg robustness

The index is stored and queried by block hash. Each entry is independent (nothing cumulative). So I think it’s fine to keep reorged blocks around. But there should be a test to make sure the new block is added.

Sjors commented at 8:07 am on September 29, 2023: member

I posted a short wish list of helper functions on the base PR, to make the index a bit less aware of low level protocol details: #28122 (comment)

fjahr commented at 3:16 pm on September 29, 2023: contributor

I am still confused why we need to let users decide which version of the index they want to run. What is the motivation to let users run an older version of the index explicitly even though they have a newer version available in their node and the newer version isn’t missing anything? I don’t see a good reason for it aside from maybe storage space. So then I think the index should be either on or off and it will be updated with every softfork to accommodate for the new types once the softfork activates. That way there is also no reindex needed if users upgrade their nodes in time. We could do explicit versioning in this scenario if it helps to simplify things I am not sure it’s even needed.

And if the storage argument is really something that matters, then I don’t see why we wouldn’t treat it just like blockfilterstypes. It seems much simpler to have separate indexes for each of these new softforks/silentpayment versions rather than deal with all the compatibility stuff inside a single index. That would also make sense in light of the storage argument: If I am just starting to use silent payments with segwit v5 then I don’t care about the silent payments that were maybe done before on older segwit versions and I could save a lot of space if I can just run -siltentpaymentindex=5 or so.

EDIT: Is it an issue with the second suggests path that senders can theoretically send to any version after taproot and the receiver can not control it? In that case I guess the first path makes even more sense because then there is really no reason for receivers to run with an older version of the index.

Sjors commented at 4:36 pm on September 29, 2023: member

What is the motivation to let users run an older version of the index explicitly even though they have a newer version available in their node and the newer version isn’t missing anything? I don’t see a good reason for it aside from maybe storage space.

Storage space would be the reason. Though in general I expect new soft forks to have low transaction counts and therefore not much extra storage. So maybe that’s not a big deal.

The other question is how to serve older clients. That might involve keeping one version entry per block and then joining them as needed. But that doesn’t require versioning at the -silentpaymentindex level.

Sjors force-pushed on Dec 11, 2023

Sjors commented at 11:48 am on December 11, 2023: member

Rebased and added a simple functional test, which includes a (hopefully) deterministic tweak.

DrahtBot added the label Needs rebase on Jan 26, 2024

josibake commented at 3:04 pm on January 26, 2024: member

@Sjors I’ve made some on #28122 to reflect the changes in the BIP + a few naming cleanups on the functions. Should be good to go for a rebase here, if you’re still working on this.

Sjors commented at 9:39 am on January 29, 2024: member

Will do soon(tm)

josibake commented at 7:24 pm on January 31, 2024: member

I was tinkering around with this and had a few thoughts:

Being able to specify a start_height sounds like something that would be generally useful for all indexes, not just this one. I’ve been toying with the idea of what it would take to have a “pruned” txindex and I would need to be able to start from a specific height for that. @fjahr I took the branch you started and got it working with a little tinkering here: https://github.com/josibake/bitcoin/tree/add-start-height-to-base-index . With a little more work this could probably be it’s own PR. Feel free to grab this branch, or I can open a PR if you’re busy with other stuff!
I wanted to see how difficult it would be to make this as a new BlockFilterType, and it was way easier than I thought it would be. The blockfilterindex code is really well designed! I copied the SP logic from this PR and got a hacked together working branch here: https://github.com/josibake/bitcoin/tree/implement-bip352-blockfilterindex (this syncs on my laptop in < 1 hr on mainnet and ends up being around 1.5gb on disk)

With the blockfilterindex, all we would need to do is propose a new filter type BIP and everything Just Works (TM) for light clients that support BIP157. I don’t see any easy way to “prune” this index to only have data for the UTXO set.. but I also don’t see a way to easily do that with a general index like we have in this PR either. I’m also less convinced this matters (for right now), and more worried we might be prematurely optimizing at this stage. @Sjors brought up some good points about versioning, but I can see that working with a block filter index in that we would just propose a new filter type for new version types (fine, so long as we don’t frequently update silent payments versions), or if we do find ourselves needing to frequently update the version we just build versioning into the encoded filter e.g. return data for all versions and let the clients figure it out, or split up the encoded filter into sections with version bytes?

Curious what you guys think on how to proceed, @Sjors @fjahr. In my mind, it makes more sense to go with a custom index if we have a clean way to support pruning to the UTXO set, and we’d also need to think about how to make this index available to light clients. If instead we go the blockfilterindex route, we get all the infrastructure for supporting light clients for free (and fast wallet rescans), but probably more difficult to build in custom logic like SP versions and UTXO pruning, since the blockfilterindexes are somewhat rigid in how they are built.

Sjors commented at 1:26 pm on February 1, 2024: member

With a little more work this could probably be it’s own PR. Feel free to grab this branch, or I can open a PR if you’re busy with other stuff!

If you’re feeling momentum, best just open a new PR. I’ll then close this one. I can always take back the baton and reopen.

josibake commented at 1:50 pm on February 1, 2024: member

With a little more work this could probably be it’s own PR. Feel free to grab this branch, or I can open a PR if you’re busy with other stuff!

If you’re feeling momentum, best just open a new PR. I’ll then close this one. I can always take back the baton and reopen.

I was referring to opening a PR just for adding start_height to the indexes (the work that @fjahr had started), but I’m also happy to open a blockfilterindex PR! Although, curious if you had any thoughts regarding the rest of my comment re: pruning to the UTXO set and versioning?

Sjors commented at 2:20 pm on February 1, 2024: member

@josibake I’ll get back to you on those later.

Sjors force-pushed on Feb 12, 2024

Sjors commented at 12:19 pm on February 12, 2024: member

Rebased.

I agree adding start_height could be useful. I grabbed the commits and set start height to the taproot activation height. Will leave it up to someone else to make it a PR.

Looks like I’m using it incorrectly though:

02024-02-12T12:17:04Z silentpaymentindex thread start
12024-02-12T12:17:04Z Syncing silentpaymentindex with block chain from height 0
22024-02-12T12:17:04Z ERROR: Commit: Failed to commit latest silentpaymentindex state
32024-02-12T12:17:04Z ERROR: UndoReadFromDisk: no undo data available
42024-02-12T12:17:04Z *** ThreadSync: Failed to write block 000000000019d6689c085ae165831e934ff763ae46a2a6c172b3f1b60a8ce26f to index database
52024-02-12T12:17:04Z Error: A fatal internal error occurred, see debug.log for details
6Error: A fatal internal error occurred, see debug.log for details

The compiler is also warning about the block filter index:

0In file included from init.cpp:30:
1./index/blockfilterindex.h:32:15: warning: non-static data member 'm_start_height' of 'BlockFilterIndex' shadows member inherited from type 'BaseIndex' [-Wshadow-field]
2    const int m_start_height;
3              ^
4./index/base.h:110:15: note: declared here
5    const int m_start_height{0};
6              ^
71 warning generated.

If instead we go the blockfilterindex route, we get all the infrastructure for supporting light clients for free (and fast wallet rescans), but probably more difficult to build in custom logic like SP versions and UTXO pruning, since the blockfilterindexes are somewhat rigid in how they are built.

Not sure about this either.

josibake commented at 12:24 pm on February 12, 2024: member

Interesting, your first error seems related to UndoData, which start height shouldn’t affect?

The compiler is also warning about the block filter index:

Ah, this is a shadowing error, I can fix that. I think I’ll go ahead and open a start_height index PR since this seems generally useful and I’ll fix the error there. @fjahr adding you as co-author unless you have any objections?

Sjors commented at 12:35 pm on February 12, 2024: member

It’s indeed a strange error. I nuked the existing silent payment index before running, so not sure what’s going on here.

(I’ll take another look once you open the start_height PR)

DrahtBot removed the label Needs rebase on Feb 12, 2024

fjahr commented at 8:58 pm on February 17, 2024: contributor

Being able to specify a start_height sounds like something that would be generally useful for all indexes, not just this one. I’ve been toying with the idea of what it would take to have a “pruned” txindex and I would need to be able to start from a specific height for that. @fjahr I took the branch you started and got it working with a little tinkering here: https://github.com/josibake/bitcoin/tree/add-start-height-to-base-index . With a little more work this could probably be its own PR. Feel free to grab this branch, or I can open a PR if you’re busy with other stuff!

Hm, I don’t think it’s clear that it’s generally useful. Not starting from the genesis block doesn’t work for coinstatsindex because it’s accumulating values from genesis on. I also don’t think it works for the current blockfilterindex because the headers build a hashchain similar to block headers. txindex would work in theory but I think we would need to check everywhere where it’s used to ensure error handling is consistent for this use case, because that would then mean just because a tx isn’t in the index doesn’t mean it doesn’t exist which is probably implied right now.

Each of these could be fixed if we had a way to get the starting point but I think it would be pretty intrusive also for coinstatsindex and blockfilterindex to handle not having values for certain blocks due to a specific start height.

In terms of usefulness for a next blockfilter type, I don’t think it’s a strong argument right now because nobody is proposing a new blockfilter type and if somebody proposes it, it’s also possible that it might be interesting to have it from genesis. And then there are also conceptual questions: If we have a new filter type for a new type of script that is introduced in the future it makes sense to not have a filter but maybe we still want to have a header chain with empty filters up to the block where it starts to get interesting (for security reasons)?

I would rather keep this scoped to silent payment index for now because there is a clear use case and I don’t see any pitfalls like with the other indices. It can be a separate PR but I would suggest calling it a precursor to this PR.

in src/index/silentpaymentindex.cpp:24 in ce0e59e4bc outdated

15@@ -16,6 +16,7 @@
16 #include <hash.h>
17 
18 constexpr uint8_t DB_SILENT_PAYMENT_INDEX{'s'};
19+const int TAPROOT_MAINNET_ACTIVATION_HEIGHT{709632};

fjahr commented at 9:11 pm on February 17, 2024:

Why hardcode this value here when we can get it from chain params?

Sjors commented at 8:45 am on February 19, 2024:

Because I want to drop that in #26201. Though perhaps Silent Payments itself is a reason to keep it. But we could still demote it to a regular constant rather than consensus.

Sjors commented at 9:02 am on February 19, 2024:

(though I think the specific reason here was because I was in a hurry)

fjahr commented at 12:20 pm on February 19, 2024:

Ok, I see. If the taproot activation height is removed we might decide not to use that specific height at all but rather one that is around the time the BIP was proposed or even to some time around now? The specifics of the approach have been changed a lot and so even if someone already has experimented with it in the last few months the current iteration likely won’t work for them to be able to spend their funds, no @josibake ? Had we discussed adding the start height as a parameter? That may be another option, though raising the complexity quite a bit…

in src/index/silentpaymentindex.cpp:97 in ce0e59e4bc outdated

94@@ -93,17 +95,11 @@ bool SilentPaymentIndex::GetSilentPaymentKeys(std::vector<CTransactionRef> txs,
95 
96 bool SilentPaymentIndex::CustomAppend(const interfaces::BlockInfo& block)
97 {
98-    // Exclude genesis block transaction because outputs are not spendable.
99-    if (block.height == 0) return true;

fjahr commented at 9:58 pm on February 17, 2024:

Removing this is causing an error on all non-mainnet chains now because m_start_height is 0 by default, which is the genesis block and it needs to be skipped because it’s unspendable and has no undo data.

Sjors commented at 9:05 am on February 19, 2024:

Brought it back.

in src/index/silentpaymentindex.cpp:34 in ce0e59e4bc outdated

28@@ -28,8 +29,9 @@ class SilentPaymentIndex::DB : public BaseIndex::DB
29     bool WriteSilentPayments(const std::pair<uint256, std::vector<CPubKey>>& tweaks);
30 };
31 
32+// TODO: check if we're on mainnet, otherwise start_height=0
33 SilentPaymentIndex::DB::DB(size_t n_cache_size, bool f_memory, bool f_wipe) :
34-    BaseIndex::DB(gArgs.GetDataDirNet() / "indexes" / "silentpaymentindex", n_cache_size, f_memory, f_wipe)
35+    BaseIndex::DB(gArgs.GetDataDirNet() / "indexes" / "silentpaymentindex", n_cache_size, f_memory, f_wipe, /*start_height=*/TAPROOT_MAINNET_ACTIVATION_HEIGHT)

fjahr commented at 0:23 am on February 18, 2024:

This isn’t the right place to pass the start_height, this is actually setting the obfuscate option of the DB that is constructed here.

fjahr commented at 0:27 am on February 18, 2024:

The start height goes into the BaseIndex constructor on line 45 below:

0SilentPaymentIndex::SilentPaymentIndex(std::unique_ptr<interfaces::Chain> chain, size_t n_cache_size, bool f_memory, bool f_wipe)
1    : BaseIndex(std::move(chain), "silentpaymentindex", /*start_height=*/TAPROOT_MAINNET_ACTIVATION_HEIGHT), m_db(std::make_unique<SilentPaymentIndex::DB>(n_cache_size, f_memory, f_wipe))
2{}

Sjors commented at 9:12 am on February 19, 2024:

Done

fjahr commented at 0:31 am on February 18, 2024: contributor

For me, the silentpaymentindex now synced fine from the Taproot start height when I applied the suggestion fixes and removed the blockfilterindex commit.

Sjors force-pushed on Feb 19, 2024

Sjors commented at 9:12 am on February 19, 2024: member

Applied @fjahr’s patches and running the indexer again…

As of block 831,110 the index is 1.7 GB (down from 54 GB #28241 (comment))

Sjors commented at 8:51 am on February 20, 2024: member

@josibake I’m trying to rebase, but I’m not sure how to change the following:

0const auto tweak_data = wallet::GetSilentPaymentTweakDataFromTxInputs(tx->vin, coins);
1
2    if (!tweak_data) continue;
3
4    const uint256 outpoint_hash = tweak_data->first;
5    CPubKey tweaked_pub_key_sum = tweak_data->second;
6    if (!tweaked_pub_key_sum.TweakAdd(outpoint_hash.begin())) {

It looks like GetSilentPaymentTweakDataFromTxInputs is only used by test code. Am I still supposed to use it, or is there a new approach?

josibake commented at 10:15 am on February 20, 2024: member

Am I still supposed to use it

Yep, still the correct function. The return value has changed, and we’ve removed the pubkey.TweakAdd method in favor of a dedicated routine in module/silentpayments. I need to expose the routine for pubkey tweaking in src/bip352.h and then you should be good.

in src/index/silentpaymentindex.cpp:67 in 16cf1d4d49 outdated

56+            continue;
57+        }
58+
59+        if (std::none_of(tx->vout.begin(), tx->vout.end(), [](const CTxOut& txout) {
60+            std::vector<std::vector<unsigned char>> solutions;
61+            return Solver(txout.scriptPubKey, solutions) == TxoutType::WITNESS_V1_TAPROOT;

theStack commented at 11:59 pm on March 4, 2024:

Considering that we only want to check if one of the outputs is P2TR, using Solver in this situation seems to be a potentially expensive tool (it does the matching for all standard output types and returns data that we never need). Thinking of e.g. txs that have dozens or hundreds of bare multisig outputs, this could be much slower. I’d suggest to use a simple fast-test here instead, e.g.:

0            const CScript& spk = txout.scriptPubKey;
1            return spk.size() == 34 && spk[0] == OP_1 && spk[1] == 0x20;

(could introduce this as CScript method IsPayToTaproot, very similar to the already existing IsPayToWitnessScriptHash)

Sjors commented at 8:42 am on March 5, 2024:

Would be nice to have this as a helper method, as imo it’s too low level for an indexer.

Sjors commented at 2:00 pm on April 25, 2024:

@josibake can I get a helper function along the lines of:

0bool MaybeSilentPayment(CTransactionRef tx);

setavenger commented at 10:12 pm on March 5, 2024: none

@Sjors I finally came around to upgrade my implementation for the tweak data. I have indexed a couple hundred blocks now which we could compare if that’s still relevant for this PR.

Sjors commented at 10:53 am on March 7, 2024: member

@setavenger yes that would be useful! However, I’m waiting (?) for @josibake to give me some helper methods so I can update this PR. Otherwise I don’t think things wil match.

setavenger commented at 9:39 pm on March 7, 2024: none

Ok, in that case you can just ping me when your ready to compare.

setavenger commented at 2:07 pm on March 8, 2024: none

@Sjors I was looking through your implementation of the index, I’m curious if there’s a segment dedicated to transaction cut-through. I couldn’t spot anything. My implementation does remove obsolete tweaks as suggested in the Appendix. I guess that would be something we’d have to keep in mind when comparing the indices.

Sjors commented at 6:23 pm on March 8, 2024: member

I have not looked a transaction cut-through. This would heavily depend on how we (later on) serve the index over p2p. If we use a message pattern like BIP157, we could perhaps decide to restrict the query ranges that are allowed. E.g. only modulo 1000 blocks would be a valid start and stop. Then we can use that fact to cut-through the index itself. (and serve the last <1000 blocks without cut-through)

We should avoid calculating this at request time, as that would probably require too much CPU use.

setavenger commented at 3:20 pm on March 9, 2024: none

I’m not quite sure I’m understanding why one would keep the obsolete tweaks (even for the last <1000 blocks). I do agree that the overhead would be too much if computed at request time. In my implementation I throw out the obsolete tweaks during indexing. That way it’s easy to serve the smallest possible array of tweaks to a light client for every block.

Sjors commented at 9:41 am on March 11, 2024: member

I throw out the obsolete tweaks during indexing

That might work, we’d have to keep an eye on how much extra disk I/O that is compared to only doing it every 1000 blocks.

Looking at this again, doesn’t cut-through remove transaction history? So then you’d need to keep two indexes, one for regular use and the other for a wallet that needs to rescan. Or is there another solution for that?

setavenger commented at 10:09 am on March 11, 2024: none

I throw out the obsolete tweaks during indexing

That might work, we’d have to keep an eye on how much extra disk I/O that is compared to only doing it every 1000 blocks.

I can’t really comment on the I/O implications, I haven’t looked too much at those yet. Especially because I’m not running my implementation on the node itself.

Looking at this again, doesn’t cut-through remove transaction history? So then you’d need to keep two indexes, one for regular use and the other for a wallet that needs to rescan. Or is there another solution for that?

The way I understood cut-through, is that once all taproot UTXOs for a given transaction are spent, the tweak for their transaction is removed from the index. This does mean that if a light client rescans, it will not find its own spent transactions. I remember there being a discussion some time ago, and I believe it was seen as a reasonable trade-off to not find ones own transaction history in favor of saving a lot on bandwidth and computation (maybe @josibake can chime in here). But yes, as far as I know, one would have to keep a full index in parallel or set a flag on spent tweaks in some way.

As a note to that. I have tested a bit with a mobile wallet and it’s already very time consuming to do a full scan (and that was just signet). From what I’ve seen cutting out unnecessary tweaks where possible is very desirable for weaker devices.

cygnet3 commented at 11:43 am on March 11, 2024: none

The way I understood cut-through, is that once all taproot UTXOs for a given transaction are spent, the tweak for their transaction is removed from the index. This does mean that if a light client rescans, it will not find its own spent transactions. I remember there being a discussion some time ago, and I believe it was seen as a reasonable trade-off to not find ones own transaction history in favor of saving a lot on bandwidth and computation (maybe @josibake can chime in here). But yes, as far as I know, one would have to keep a full index in parallel or set a flag on spent tweaks in some way.

It should also be noted that some wallets may want to continue tracking the scriptpubkeys that were used for earlier transactions, in case users screw up and accidentally send to the same on-chain address twice. That is not part of the silent payments protocol, but it could occur if users are trying to re-send to an address that they see on a block explorer.

So if wallets want to track these scriptpubkeys, they would also need to be able to discover them even after they are spent.

Then again, you could argue that this is a niche wallet recovery scenario, and if users want to do this, they shouldn’t use a light wallet.

Sjors commented at 11:55 am on March 11, 2024: member

and if users want to do this, they shouldn’t use a light wallet.

It’s the recipient that uses a light wallet. They can’t control mistakes by the sender.

However this issue is not related to the index. The index just returns tweaks, the wallet then needs to generate the scriptpubkeys to monitor. For that it would use BIP158 “neutrino” filters. The wallet should never remove a used address from such a filter. That means the filter will grow, but I don’t think that becomes a practical issue (high false positive rate, more blocks to download) unless you receive tens of thousands of transactions - at which point you may want to switch wallets.

(the spec should probably say something about this, if it doesn’t already)

setavenger commented at 12:38 pm on March 11, 2024: none

However this issue is not related to the index. The index just returns tweaks, the wallet then needs to generate the scriptpubkeys to monitor. For that it would use BIP158 “neutrino” filters. The wallet should never remove a used address from such a filter. That means the filter will grow, but I don’t think that becomes a practical issue (high false positive rate, more blocks to download) unless you receive tens of thousands of transactions - at which point you may want to switch wallets.

I guess it does become relevant if a tweak index implements cut-through. A light client that is completely restored from seed and has to do a complete rescan would be unable to find UTXOs that were re-sent to an address. The new tweak for that tx would not match the scriptPubKey, the old tweak would be gone and the scriptpubkey would go unnoticed because those would not be available in a recovery from seed.

I agree that this case should be considered in the specification. Or at least a discussion for light clients would be good in order to think about these kinds of trade-offs and pitfalls. Has there been a discussion in this direction already?

DrahtBot added the label Needs rebase on Mar 11, 2024

Sjors commented at 3:46 pm on March 11, 2024: member

I think that for early development it makes most sense to have two indexes: one without cut-through (this PR) and one with cut-through (maybe a followup?).

For the purpose of testing equality, is it straight-forward for you to build an index without cut-through?

The recovery use case is less common, so in the long run it would be fine if e.g. most nodes run the cut-through index and only a few run the full one.

setavenger commented at 4:50 pm on March 11, 2024: none

I think that for early development it makes most sense to have two indexes: one without cut-through (this PR) and one with cut-through (maybe a followup?).

Agreed. I could do both as well. The extra work should be minimal.

For the purpose of testing equality, is it straight-forward for you to build an index without cut-through?

I think it should be quite straight-forward. My implementation might even benefit from having the two options available. Just to be sure and we get the same results, you are only collecting the actual 33byte tweaks per block/blockhash, correct?

Sjors commented at 5:20 pm on March 11, 2024: member

you are only collecting the actual 33byte tweaks per block/blockhash, correct

What else would you have in mind?

setavenger commented at 6:51 pm on March 11, 2024: none

you are only collecting the actual 33byte tweaks per block/blockhash, correct

What else would you have in mind?

I’m not sure, was just thinking, because I’m collecting a bit of periphery for the tweaks (e.g. txids, and more) but I guess that’s not really relevant here. Thinking ahead though, how exactly would you want to compare the tweaks, sorted and hashed I presume? I have a feeling it will be easier to store the data already sorted instead of having to do that afterwards.

Sjors commented at 6:55 pm on March 11, 2024: member

I haven’t studied this PR in a while (and my memory is terrible), but I believe we store one entry per block. The entry is an array with one element for transaction, which is either empty or a tweak. The RPC similarly asks for a block hash and returns an array where each element is either empty or a tweak.

setavenger commented at 7:31 pm on March 11, 2024: none

In that case we could maybe just compare a hash of all tweaks per block, with the tweaks sorted lexicographically.

Sjors commented at 8:39 am on March 12, 2024: member

@setavenger it’s much easier to debug if we just compare all tweaks individually.

setavenger commented at 9:31 am on March 12, 2024: none

@setavenger it’s much easier to debug if we just compare all tweaks individually.

In that case it could help having the corresponding txid for a tweak, what do you think? Or are you able to accurately track a tweak in a different way?

Sjors commented at 10:12 am on March 12, 2024: member

The RPC returns an array for each block with one entry per transaction. The entry is empty if there is no tweak. So the ordering is based on the order of transactions in a block.

setavenger commented at 4:12 pm on March 12, 2024: none

Ok, then I’ll make sure my full index follows the same pattern.

setavenger commented at 11:47 am on March 14, 2024: none

@Sjors I was able to run the full index but due to performance issues I couldn’t keep the tweaks in order. I jotted down a couple notes regarding the tweak index, light client implementation etc. here. This might be interesting for other implementations as well. I also published the index and some peripheral data there.

A couple key stats:

index ranges from 709632 -> 834636
my full index is 1.7 GB (https://github.com/bitcoin/bitcoin/pull/28241#issuecomment-1952006804) so we are in the same ballpark. I believe I’m using the same method to store the index data
cut-through leads to a reduction of roughly ~40% in the number of total tweaks

Sjors force-pushed on Apr 17, 2024

Sjors commented at 8:43 am on April 17, 2024: member

Rebased. It no longer has a wallet dependency, which is nice.

The RPC returns an array for each block with one entry per transaction. The entry is empty if there is no tweak. So the ordering is based on the order of transactions in a block.

The middle part here is wrong: when a transaction doesn’t have a tweak it’s simply skipped, there’s no empty element. I could change that, but I don’t think it’s necessary for anything.

Here’s the tweaks I get on a recent signet block:

  0{
  1  "tweaks": [
  2    "03dde7131aedd4ffb7f0c8190253c61d5c268d2d9bf495b3e8f70161c4a88748b1",
  3    "02d69b26c5e219c13ba258600ce2d88adb604cf9e3dbec9a4cba3d79072fc35aec",
  4    "03cf8f34abd0f4cc6e2804b2cf8afd564846433f135af3678c9d7bd03eacf12edc",
  5    "025a08ce8e8d85765f91ad10af65755cf3c60dd770b3d72384786134765d69820b",
  6    "02e3f1656ec39e4240fffa6f36ff2eb57f20e7f819340cf44317690990ddc98544",
  7    "03e674eb283627a44cf4aab3ba50651d1093e732f94baaca2d74c96ccaa48ca8b2",
  8    "026c0973324533bf2b9d60d6ff8ad133493ee8828462a476ab0c2df171bbd0c2b1",
  9    "02199bca98e3da7687680d01ecfd9075db1e45d2a00a2a4c4981663a99e3b2ff10",
 10    "03ef64b3387938515ac203493cebc5c21a62415e7bc987542fda269add11854713",
 11    "0263e6534d7e3968d14dbb4d5710b7077902963e6f6b4138b4d7f1f611465132dd",
 12    "02688fd83b707668c040d348dd64fed5a5000448bb00d57aafe5ee9b11f0447ffe",
 13    "039d07d18475ae86ac8ac3c87fcf31bc4c254524fff840822b76eb1613e84db499",
 14    "0202f81b34ba6a4898dd5095f3e9b9a7ed8fb88401834fab61b63a6bdc436887bb",
 15    "02c0d7686d33428e67e0fbda243dfca566d6d5905b0ad08cf1d0d35fd084ea7773",
 16    "03e3da7b3d62397a6b98fdd07590a0c846b13d5bfe314a8a077f3cd77f5c40a84b",
 17    "03d7d59345bb9ec837014e52c4c9ef5fee1e3b33de5772e3fd20e186de977dc192",
 18    "023a6cc21ae1fa66091d96629170b57cba644b10f192df85f879278f0d02fc3923",
 19    "0370c79353f62462295b2493a0eadfa01204a64a03660731e937cc256cea041f49",
 20    "02df8f5e4b7ef17de157523fd32c5cc48711f3b036dc3973b82c7c8fae24f96563",
 21    "0200797c52d48f366ee4551428e353e3df3a9456bd957c534f7db11704ae35f0f3",
 22    "036f6617e4e8e8336c3011611127fcf8bb59467af09ce03081b82b45bd522f7e88",
 23    "03e8fb68156fc78a70c3895b1529625cfc8a8c93a7977701a6aca9051bf1215348",
 24    "02664b42446ba19da313adc6a8319942edec22956d1ffe0d0f3753e26a94bcc06c",
 25    "036eedc78d6eecbe2def9133b8ae233e72b511b55e98a6b045586e19412a1e718e",
 26    "02f391d95a701cf916bd8902ce30bebf4b3517f32bc0adaf28ea7daaefa223a921",
 27    "0229fcfc868dae34eb8cd6bf9401a5c5d6304e51e50c288a8923591e97bdcc0733",
 28    "02dffebc4193b1e7a103abdc61d13af33a04330dc0d27eb9de5db6e2f3fa73cd8e",
 29    "03d83568533fe290f056abb888594c53157233ff9cadda9b09704bbbc5474f8c54",
 30    "033e1155156b29b10b3faef395bbc107df30a53d6d2e1945ad0e7f19fcbd7df133",
 31    "021c51d6dbb9c5c3db635175d25967c09d6b8c94ba352d55655ead1a3ba5bad34d",
 32    "02bad2eee846e43b1c3c0f0ada5ab2eb51b5b55f6445a627749d366a702323ae22",
 33    "02df04c0be57db13589c2472abd61e3001ce2769ac839a32c3f76d0f66a5e10408",
 34    "02665859519d38fe4e226839679c355b2ff2c2cf3c72c08e9fa35889dd66c9274e",
 35    "02962c2eb048f47dda56a4c839199f2d1e567435fee68268372f6e4c01f8b7a86a",
 36    "022184a3ce6d63dbce433bfca44a8596d7ed035b42afa3a1e9782396838cfa01de",
 37    "03fd1043acf8a35297c204df2612d03843979cac685c5ac7ce53683fe554cb6879",
 38    "02cb1a084af2690fd06cac6da519ed100a5c44c8af27ade9d2c7b37e900af3a948",
 39    "03585571e85609eb1378ce53c517650c32635ee68685ce6ca62b98662ca7d97f1b",
 40    "03d24b172c6a3be7b7bb0946c75703532a80adea7d94c9b605979efb9c6d3ef823",
 41    "034b46840cbf6500d7a82d1bd397dbed098aa230fa1915e1b28921db967fe2334a",
 42    "029d9176ea61a1c45d8560ce897f9f0b46d408096d46b2dcedad9c039824e1ad49",
 43    "039419755c7f70a486e3a7470976f7a70cb77ba26620869501027484135ef6c48e",
 44    "02f590d701a5559feb5d672d69f92789d3cd13304408b36f9686f44abe4256086d",
 45    "02cd8c3a992fab9883ec88666d4c2232f2a3007c685393be23e7aa9fc58c3216e8",
 46    "0388c4a9e53407470834ccfde64bb76345aa9bdf84c32da0b74b3ba8e390652ce0",
 47    "0361aedc2bda5fd14ec038425980c69253624c733c4d99992eeb569bd1aa58c85d",
 48    "0280473683c77bd3185585252125b8a9669741095e033eae1462d9f35e4667da27",
 49    "03d387b416d93f0bb785e8ba31cb3053a587fa28be11ae44b5b065697e40417bef",
 50    "03c45748c66ff5e76f27a9ccd17c1305c599266e36751fb8c44e1b1d6de1c947d0",
 51    "02290446b0263057ac9fba6a277c4244a482b2238ba2c415b181975977cdd33281",
 52    "02bdfd35682562719cb4a26428dbd9a38ec19cc659e280f33b565c4cc06f1f4224",
 53    "026ffad185910b1e5e2e74320e5649a5ccec25807b20626d140a09ef87e8492f38",
 54    "0328521c4b2363a64fd5679efd71fad2c7d857c34ae41b46c0230a0f9220e6b3f7",
 55    "03a3dc0aff3941e2d7c7d3a69ca5ba2afc35fa28df2a17f3e7d35d19411dfddc55",
 56    "0262b5193d4fb7470a065f1444979ca18c0b4ed6a8d730c852a4c6650c30850594",
 57    "025c3c5602bb5d50c8fa1d287b9c6698b28f3c1697183377c3654985a931108cf1",
 58    "03d3574d15a6b9a069cde1c570c93ee6d94eeacc63f1426434385b1319afaadaa8",
 59    "020d1efc22d6aff6f49a02e2894cee64a6d03bbd804a89e0809eaf0a44f657b97b",
 60    "02035b7e10e5062f1ff7ac98a5ad26ceb8854de45197fe429d61eb86d6db04f749",
 61    "029bd0d8a9f3db6ed4e47a5b100b45e0373a988c5710d63fedfff68809c9c9ba30",
 62    "02f221e959bbb5fed1f6a8cc55bec48bf1f0c398170954d2181cd0c3fb7080265e",
 63    "025d70330b90e4da34e893bab4de988620393be736fe74d5a97fe7f296eb240299",
 64    "02445cd8e53dc46e4c7dc04d9778376a153d57f27884efba7f7f034cad85544194",
 65    "02fd9ede367839371a15ccb3933ba5717bca9b38e759bbf2952061d3a32bf254db",
 66    "03a3fa126a9af15bc0354a6c87a453d42c77472f3bfd8d76d599e2ad4f1187f437",
 67    "030c822f92dad97ef49af0383f492f1f79c9e7bb522f363e1de4b231fa36ee26d6",
 68    "03cfa37488e3ec657cbaaa3be8802af238a1c827c9bbf61045aa45dbf4d00a2055",
 69    "0356532eebb2bc3b764ee2a34d9ff4a1797b843ce1eb194fca7f3c6a1a60b26111",
 70    "02d6e21027cec1e5eb4dac6b4be44f0601398886acd1a8ea3aa99d99bfef52d4d8",
 71    "03bff341819a679295fc159c6d78d535f6d26e79ed28b6045979828449d3d81404",
 72    "039f7ea2f399a9fdc4c9a5fdd99bd41285e829eb320088c18e565bff5bf7f490f2",
 73    "02f977a4159d25d9c0197c614a653ef549a94088a12f49dd71c4fa32877287d65e",
 74    "0398a9d825c49626dea0f4371863519b86e75f4c951aac87cb1f8c621ba23cfd38",
 75    "028278c78676e88f13dfa7efe5351eb3951125313a0b7a6c18274a65e0b0f734b7",
 76    "033ff8f7f4afeb45730a4ba47145b1d3f628179cae5d7c185cfad9971a82c1a08d",
 77    "031405054061d9fb120a785e0dbbdb3bbf5353e99254e11a5e9ece6eca77393d88",
 78    "034e27e90802aab13af0340246eec9a30337e50da8a59067371dd2362efbbc3ec2",
 79    "032e19d8915d22d537ebe4c7ba5532a54ad68fbea0b9bcc7431892b0816abd91d7",
 80    "03ade6ae261ba9e9f4bfb8e02f6b26e7d23b9d06e9f1423681c9117c034892187f",
 81    "029dc032b8faabf9b3c7c54c83ca3c7f5f7be0a72b4f01e6b7c9de4796fd0af702",
 82    "0302062e06ff2d1fc971c885d610c3c192053600e71ff04aedd79fd9b8462483a4",
 83    "036d39837fa8f15fbd1f42a3041ffed5ddc18f5569cb4ab7de2acf338fba10badc",
 84    "030997ee943e289f82e0b2a3cabfa5695e54c834886d336cb395dfce4222f24891",
 85    "03c262abff0b89b764854e0ece12761bb728d4ec4d654535d1d0da5eb30733ca1e",
 86    "02b80cc210cb9301d5b538262cc022d553b1eaf9f5324f1e77644960020ce8d47a",
 87    "035abc895c2a93a0cff15b05f7700e8b966adc902a14f9c04d303d98e435a2e038",
 88    "021e2831164d40726addfd1dbce18a45823a729ecb5de448173f2de7a972b3c699",
 89    "030e9e9e650b663b45bc19bc919bbad47b27ee7517956fd3af91a5060ef18940c6",
 90    "0383e6f14e9ce6fee2703dca908109222ac314ef55e10cd40225965e7834ef9ae3",
 91    "02d3e54b107d61f41f7e075b42ea1f3cda346a9533b513d0ba7ae414340f6bbd2b",
 92    "024b56519418825efce7e8461f1b89dea87512702fc1e7f20594410db2ab44e4b0",
 93    "027ad45dfe13d32e6245d6ff5490388153be629c6ee54bb5b71803985465844aa5",
 94    "027f9a01694a7db47a75e43900dd65a7864c3b221b723f3c26c072369a8a3e2bb7",
 95    "028d3b965a00e51246f38f97a18d43ef72c6f514681efb2b8db41f675f0b39e6a4",
 96    "02ea5eea75dc7fdcc09a09d18f4d2bad9b10143c74372731dc00ac4719271e3b9e",
 97    "03ee4480f1457043cd79cd5e253ac44f2fe1b52fa6d7c327b9ff835ce543c3abf6",
 98    "02bb886ae36426b0f7e0ef14aaba151a0f700652e3e21b7f57e74ff32536158d6f",
 99    "022117a2ce70f9db5c4ac264f31fe344d1ba0a794c2bade8e9bc291dff2529845a",
100    "02e7ca97bfbeac1fbdef85e2610defddd6786f4eb58475e5ff0df095d2f4453ea6",
101    "0285f650d4fec806e6560926b5b164de6b129b79b489d6790cb3da42d325322783",
102    "03b21ff0d3fcefa6435a093c6932a1aac9cebb5118bc69865855d95ea293da829d",
103    "039d7c959a684a92117e8da302c3141a99ac942aea3cd2a99762ec00eaceeef8e7",
104    "0348759e66fc521108075043681fa0be4f5906ccaa18e4111a9bbea0f8aaf86159",
105    "03978a366bf1b745d1f9e6c7df10566439ee010542461356ac9f0833a3bf74851e",
106    "028e633daca005532b44df046bfded6a5e69eb06badcb609f990b9d2e3b0793c5a",
107    "0308708b9e27944d9de82ed9cfeac51681455f4f1584869c2cdb73ba1e0f71d0b1",
108    "02c7f36661ce2bb478ddbdeb927a56cff5628c4ab17280ddd8b0791c39067838f7",
109    "023b6de613085e662bdc681de94092704e6c114bcb08c628ac066b2ad8f34b186a",
110    "0322033f1a4dbedf951eec5454f9f18cedca7ff3c7823fca1203ebfb91a6b7cc06",
111    "02ebb434e8f8aba732b05023dcc5c13c04556febed25a5a6c5d7719c42da783b76",
112    "035a6bf473764fbc64767b27866e1ee82380247c197568ae52203266037049cc64",
113    "028a7e4a4684a3b264b5e339b5a7376ec5e0e2beaab7223bc926c6bd401f0f8a33",
114    "03bd2a3e364a767f89507392cf60f5bb7baf65df8c576dc745dce29449dbe69e7d",
115    "02100bf00d138aa9b0186a2bdc04b86f617b7db1c43ce4c6d98a6a73873909bdcf",
116    "025f663962346b24920d2046ebf6c7a6c59d62b6641b2d5f529a278463a01681e2",
117    "02cdee816b652c733ae7118760cddf5d6c224b4ddc2a60a94f9b63969f65fc8905",
118    "03e0111bfca7188a4ff597c2ab9762d40150e2d5eaa4d7b6ee0619c244f00f6e1d",
119    "03a362c507df74497a9a4c4ec1b96157c641067877448a961a2fd12fc12a57dc9f",
120    "0300f11a977e52d4b2df2f340dbb48939af569f80abcfb721d80bcaad241478dd8",
121    "0338941aca63e2e24a8ff823b9b82346c7d3b25e98a71f33c1f5aa2f44cc9456df",
122    "021bf9c708e836e05470ddaf418961e840b5bb1d5392e6cf7259cc0072a2ba8e33",
123    "039c3cd95ef1c859c27ed8c8ef0b74d362288eab068d7ed8aad38851a657dbc473",
124    "0373793ecc68a94acda07708dadf2082318f5c2586fabbacad7dadad55c87cf513",
125    "02e7436b58aab20271f0b5d5865921bfb4534b63c904034be995dfebe383184f1a",
126    "0287828b42211f00491b8c34c129e069e780c2e928800433b4753b2c82628818e1",
127    "0234400b2e0dfb8d0121c66315f210de41d989dcb8374f0649120e5206b9120d8c",
128    "02414187a6e41625c111411c3557c7c4cf79a77b7c2c90be290cb30468e21f04ad",
129    "0361aca306f5d7c3584d4554aae89fecc76886d09c87ca4ffaeb1ddcf1995f3ccd",
130    "0223d4a2975c7edc6d4ef9d0115ffc2ca301de8abc33b4e8f16c5ff6dcba68aba7",
131    "028ff059b5ead51d75905778d70505a9577c55f0dd13ba16b31c312952f794abca",
132    "03c8b14ebe61fa7f776919e4e1443558d8d453bc306728781d400f17d12d1d645e",
133    "02c4cfe62d07450117db533e8f781ccecc61fff6b3cdf4d7fd635407d0ea6104d0",
134    "025f303c19c542f50a535be71323403816950d535033b2bc7c2bbd468b95011eb9",
135    "02a2910f75512fc66310f6af63c36b5d70f9bec3b4a9502ef8021b8d0aca83a29a",
136    "02df7e5ef91d223bee7e4705e5fdd6767d259e5ee0b9b577add84c8485e59dc59f",
137    "02509825a12b1638e051903bb7bddd2e4e190595b73cc0330e6e510e2866b8d50d",
138    "0355135adc7b4cef16bbb6800bf993bf36538e823ea4f03a31e40c360e2bce52c1",
139    "03f5156fb6f04624941ba8621e889d440ff6861ad231a0979ee66f9df7c51f2dcf",
140    "0378996dd85f0c7cc2a804cc009b1b0f854df84a81510e1bd4019df0251c03c0d1",
141    "0277e0904ec2c6402843f5d6babc1f173436543ec89d1dfde7d0c104a6cd1c5273",
142    "021f10375da54be801d51be2233b1fcbd52340d08dc706a1b3e753033568767324",
143    "0262aecedfeaf0df822e019c124fd95e606ff775aa30f9a3e125a87b01b0f30eff",
144    "02d0bbb0aa14dc4f3ba2043de749ea1d63c105de0577b37f71ec9658f8a0e949c0",
145    "0325c11eaa27cb529a3392064cfb49ac1f999b5c8ff7f14917abcdad67be86ba60",
146    "02aabf4e66a7f218206975818264b946eadd3e4ff57ae41f49a4262e1b848945b6",
147    "03d2a22045b2026977059b2ec0b803d722592671e800a30c2ba47fe9c3f3705560",
148    "031f8ca3109d282190b1bd2610890a2ff13ca55710bc5596cc6ce27745f8c050ea",
149    "03288e9994b079988eb369083b3f5b4a6c31dcb90c2904d212c7d80fdbd4298b94",
150    "02aae19e60d90b9b9829b60dd851bc4c63c916a93d71f3cd709180862eab6de09f",
151    "02884c700c60d91413bdf1c8d901b876ec6b601cb13c08567e642881a7751fb6dd",
152    "02ba16b1174391da8e69038a99571afcd37821208cb1d0ca4bb8bfff4d38bac70a",
153    "032ee3d3257472b05e5b5898244db2770419be4a4fa6e0969bf5783d682ba3bb7a",
154    "022e9c923b710601c0a9e4b2f7c347c4c98daed810329722c760cfabfa3a16c79a",
155    "03f734f312f6be23104ceb723d65e157caa09506693be5e85fc590299f09461169",
156    "03312306b90116a7c6329f63a0bfda88ff75d3a707972cc1e0dfe4807189e05634",
157    "03987a966332397cc8e53afb797965a0add8745f8b3c1ee7a302b709be177b5e53",
158    "030532bd3bc68e4a433f5a2f328c0923dea81d1922a4c50cdbcfe4b98a2b7bd7de",
159    "027698978b34610a4479172849d5ee811928698572489b7f9732e4942ae3451189",
160    "0257332a323b92a04b4dea3449953bcaa0b27571fc20265a14476f64d0b5af2b9c",
161    "0354107b26fcbe2cdb69e92b454d19d3485d1d06005b3d819790504847c0e79813",
162    "036d9953598ed569e4030739bd61ef8b811c4fcd6c18377266653a2c1a46be24d0",
163    "02f66f188aa800651d8e11a8efaf3ff14606196ad26a4dbf5d020911a513186c41",
164    "023d47e6088ce2984f92752d1f3c796ff6a2d3c37419d2f0c199756bb9ecd7e02c",
165    "0384acb414a8fb2e61896018b26b0e25a73f304011bc5b6062a3e36bfb0e538cb9",
166    "02630235c2b85eb8dde983382ef0d57ae113efc555e5d5ff8dffea0ad23a4d686e"
167  ]
168}

in src/index/silentpaymentindex.cpp:86 in 4b4e99b1da outdated

81+
82+        std::optional<BIP352::PubTweakData> tweak_data = BIP352::GetSilentPaymentTweakDataFromTxInputs(tx->vin, coins, /*combined=*/true);
83+
84+        if (!tweak_data) continue;
85+
86+        // TODO: friends don't make friends do low level libsecp stuff on return values...

Sjors commented at 8:46 am on April 17, 2024:

@josibake can you make GetSilentPaymentTweakDataFromTxInputs return a CPub (or uint256) rather than this low level secp thing?

josibake commented at 4:17 pm on April 22, 2024:

Added a new function GetSerializedSilentPaymentsPublicData that returns a CPubKey.

josibake commented at 4:17 pm on April 22, 2024:

also lol at the // TODO comment

Sjors force-pushed on Apr 17, 2024

setavenger commented at 2:20 pm on April 17, 2024: none

Here’s the tweaks I get on a recent signet block:

Do you have a mainnet block before height 834761 indexed, then we could compare. I don’t have signet data available.

Sjors commented at 6:30 am on April 18, 2024: member

@setavenger here’s one for height 727505:

0src/bitcoin-cli getsilentpaymentblockdata 00000000000000000002d8cc04ec765eeeaaf4684f43c2ec28d81ad34436b1bc

 0{
 1  "tweaks": [
 2    "03031f9711550ee688c52fe8635a1b122eb2744c362e19e6b2284dd69ab8d277b4",
 3    "02be4494f8a62126483634f45192b189cd121bc037ba0b8f8f1ac4bd4e15135c94",
 4    "03c22ca4a3d3d0e0fa8073fdc7d252f599726445a56fd61523c6f5150ed151a992",
 5    "02adf5951aa06107d18f35e20d902b8b54c4e8ff82ddb91e7a265f7146c018ded6",
 6    "03b953cbc7bf3e73a87c267a0b155666ff3e0d866a35975b850ed56a193ed416d1",
 7    "0207a9a7ce03287d67984117a2301e4c6f3faf2e7559d1151fc3a22ea8ee897727",
 8    "02a395fec36901fc8f3f386ae25832808f543e5591f54d8652def3db7e29ea4121",
 9    "037d4ee4afe27b873f85930581927f0106ffda7b984c3e19ab457ce0ddfe753d86",
10    "024fe4fc3741862672359c6ac3924f641999d3bbaa0ec9ec3002675ad11b821cac",
11    "02bcc7a0f5d0ce462b7ad3fe92c7e1367ed8321b89c92bf97352c9662bc0aae721",
12    "03e2d7e33df03c6485cd97df98e74af9d64e7ef26768c01f42a5298e7bee97df13",
13    "0342cdf6ee793f46f54855441583cbbf3eb7745c49cb8c3dda095de0a83a6b73ab",
14    "03f1cddd05f526068bc2f4d39ea618daf546d1cb4eb7671554898596accd1f3c76",
15    "02b997fb340b5ea0599aebcba1c01ea7efaf4f60fb49a727e6400aee89947bda8b",
16    "024b75d6958cc6972b97d88319e5a14ccbc6b7dad8cb54bd0ff0e2d3fed8cc8863",
17    "02292c860c456731ccdab58fdc40e16a4c431680024ccbac2f5c32087fe7b851ca",
18    "021c31eaebf539ce61083717933885eef536577d6e441f94e6f7e7bfaa51f03fe8",
19    "026e8cd29797f659f13dcab968ed1be60591a1999723c1bf06daf453edf3af4f51",
20    "025a09b88764cd2a24162144c98d327b9ba34e4e4061531fe2abdeb2f8dded8053",
21    "02a67a08b1df9aa0f39e4d45c4c9d8d75267ec030099cf6c5694d840ba948d3af1",
22    "02cb9357c3d9ba8a0beb92acaf24761110cc09cb2a070bc5335668360cefe92f8a"
23  ]
24}

setavenger commented at 6:50 am on April 18, 2024: none

@Sjors Got the same results. As mentioned above I had to sort them in order to compare them.

How should we best go about comparing more blocks? I have linked the entire tweak index here. Maybe you could do something like that for a couple of blocks and we could compare the tweaks?

 0[
 1  "0207a9a7ce03287d67984117a2301e4c6f3faf2e7559d1151fc3a22ea8ee897727",
 2  "021c31eaebf539ce61083717933885eef536577d6e441f94e6f7e7bfaa51f03fe8",
 3  "02292c860c456731ccdab58fdc40e16a4c431680024ccbac2f5c32087fe7b851ca",
 4  "024b75d6958cc6972b97d88319e5a14ccbc6b7dad8cb54bd0ff0e2d3fed8cc8863",
 5  "024fe4fc3741862672359c6ac3924f641999d3bbaa0ec9ec3002675ad11b821cac",
 6  "025a09b88764cd2a24162144c98d327b9ba34e4e4061531fe2abdeb2f8dded8053",
 7  "026e8cd29797f659f13dcab968ed1be60591a1999723c1bf06daf453edf3af4f51",
 8  "02a395fec36901fc8f3f386ae25832808f543e5591f54d8652def3db7e29ea4121",
 9  "02a67a08b1df9aa0f39e4d45c4c9d8d75267ec030099cf6c5694d840ba948d3af1",
10  "02adf5951aa06107d18f35e20d902b8b54c4e8ff82ddb91e7a265f7146c018ded6",
11  "02b997fb340b5ea0599aebcba1c01ea7efaf4f60fb49a727e6400aee89947bda8b",
12  "02bcc7a0f5d0ce462b7ad3fe92c7e1367ed8321b89c92bf97352c9662bc0aae721",
13  "02be4494f8a62126483634f45192b189cd121bc037ba0b8f8f1ac4bd4e15135c94",
14  "02cb9357c3d9ba8a0beb92acaf24761110cc09cb2a070bc5335668360cefe92f8a",
15  "03031f9711550ee688c52fe8635a1b122eb2744c362e19e6b2284dd69ab8d277b4",
16  "0342cdf6ee793f46f54855441583cbbf3eb7745c49cb8c3dda095de0a83a6b73ab",
17  "037d4ee4afe27b873f85930581927f0106ffda7b984c3e19ab457ce0ddfe753d86",
18  "03b953cbc7bf3e73a87c267a0b155666ff3e0d866a35975b850ed56a193ed416d1",
19  "03c22ca4a3d3d0e0fa8073fdc7d252f599726445a56fd61523c6f5150ed151a992",
20  "03e2d7e33df03c6485cd97df98e74af9d64e7ef26768c01f42a5298e7bee97df13",
21  "03f1cddd05f526068bc2f4d39ea618daf546d1cb4eb7671554898596accd1f3c76"
22]

Sjors commented at 2:17 pm on April 18, 2024: member

Either you could generate the mainnet index using the code in this pull request, and then compare it. Or I can take your list and compare it to my result. Whoever gets around to it first :-)

Squashed 'src/secp256k1/' changes from d8311688bd..3d08027789

3d08027789 ci: enable silentpayments module
85946762a5 tests: add BIP-352 test vectors
bf349c2a08 silentpayments: add examples/silentpayments.c
9a7106e19c silentpayments: add recipient light client support
f113564298 silentpayments: add recipient scanning routine
4fb8716f4f silentpayments: add opaque data type `public_data`
987d829e8f silentpayments: add recipient label support
14ca754578 silentpayments: add sender routine
9b965927da silentpayments: implement output pubkey creation
a0fcc2c780 silentpayments: implement shared secret creation
13f203dacd silentpayments: add sortable recipient struct
a9326bdd7a doc: add module description for silentpayments
15d3e71cc1 build: add skeleton for new silentpayments (BIP352) module
cc7d18a8a8 extrakeys: add secp256k1_pubkey_sort

git-subtree-dir: src/secp256k1
git-subtree-split: 3d080277895655e8274ee73aacd154c4ead143e3

785ef3d558

Merge commit '785ef3d5588dd6975bd4e8e067e6a62697867fe8' into refresh-secp256k1 bfe9876348

setavenger commented at 9:54 am on April 22, 2024: none

Is there an easy way to run this pull-request version without compromising my existing full-node data and needing to resync everything? Then I could probably get to a comparison in the next days.

Sjors commented at 10:15 am on April 22, 2024: member

@setavenger this PR should not break your node. It just creates an indexes/silent_payments directory in your data dir, which you can safely delete later.

setavenger commented at 11:04 am on April 22, 2024: none

Ok, I see. Then I’ll be able to get to this a bit faster.

Squashed 'src/secp256k1/' changes from 3d08027789..0270b14309

0270b14309 labels: actually set the label

git-subtree-dir: src/secp256k1
git-subtree-split: 0270b1430981584582645161a04a4df67cd187bb

7607d3cfbe

Merge commit '7607d3cfbe3c86ad9d01f56437dd0a38fbb13045' into refresh-secp256k1 03a2a2d78a

Sjors commented at 10:47 am on April 25, 2024: member

@setavenger I built your index and then ran the following comparison script:

0#!/bin/bash
1set -e
2TIP=`bitcoin-cli getblockcount`
3for height in $(seq 709632 $TIP); do
4  if [ $((height % 10000)) -eq 0 ]; then
5    echo "$height..."
6  fi
7  HASH=`bitcoin-cli getblockhash $height`
8  diff <(curl -s localhost:8000/tweak-index/$height | jq 'sort') <(bitcoin-cli getsilentpaymentblockdata $HASH | jq '.tweaks | sort') || (echo $height; exit 1)
9done

The need for sort is a bit annoying, but not a big deal. Unfortunately it does find a difference at height 716,120.

This is what the RPC finds:

0[
1  "0323fefb4e4bf637d7819c2030996dbe88d97c6a2f5708841df0e65fafd32c5a0a",
2  "03cdc8c9f07d9917ed05d20231c63cac058b54a4095f6cc2fc53fc876e1226b44b",
3  "031799df7770cfdabe460e47f4571fe4ded7d7a7922afa0f5ad91753473269cdbb",
4  "03b2bda3a513123fe810cb1e65cd8a71a2495ea9229c50e4acc8d4b5baa51b4147"
5]

Versus your indexer:

0[
1  "031799df7770cfdabe460e47f4571fe4ded7d7a7922afa0f5ad91753473269cdbb",
2  "03b2bda3a513123fe810cb1e65cd8a71a2495ea9229c50e4acc8d4b5baa51b4147",
3  "030acf723f1e3bfb392ccbe0460e28be1ac13cce512c3cfe618cce839aac6212bb",
4  "03cdc8c9f07d9917ed05d20231c63cac058b54a4095f6cc2fc53fc876e1226b44b"
5]

setavenger commented at 10:51 am on April 25, 2024: none

The need for sort is a bit annoying, but not a big deal. Unfortunately it does find a difference at height 716,120.

Is that a difference so far or the only one found? I’ll investigate that block. Thanks for raising this to my attention.

Sjors commented at 11:07 am on April 25, 2024: member

@setavenger it’s the first one - will run the script again once it’s clarified (the mistake can be on our side too!).

setavenger commented at 11:15 am on April 25, 2024: none

I believe the transaction in question is this one here. There are 6 pretty standard P2SH-P2WPKH inputs. These should be the public keys extracted for that transaction:

0[
1  "031ad46f7f9242b605df6345afadd92a36c4fc4db4b393d7a55601f5b36e3bff30",
2  "02a1321db69985d13433830274b4d3fbe00192d90edab0b3297e15319d849dc13a",
3  "03c65b9db60d7b60b90a89bea66096d787f93f67fe3f5d8716b06a0f84835fcd66",
4  "02461af18902a1500606d0a5a83055307c7b54d4937b65d6959ee4535173ebc79d",
5  "0258096406d177d5d782ae27aee741cdb39d180a3dccf9ab6ecd0073e2333f8cfe",
6  "02890ed13719a603473460914e9f55998f229dee6c5bfcf862af463deba37c6b71"
7]

Sjors commented at 11:32 am on April 25, 2024: member

I’ll rebase on this #28241 (review) just to make sure and will re-generate the Bitcoin Core index and then try again.

Sjors force-pushed on Apr 25, 2024

DrahtBot removed the label Needs rebase on Apr 25, 2024

Sjors commented at 1:46 pm on April 25, 2024: member

Unfortunately that didn’t help, so the mystery remains.

setavenger commented at 1:54 pm on April 25, 2024: none

Just curious. Does the index use this to compare outpoints? Does it do (txid, vout) or (txid||vout)?

Sjors commented at 2:00 pm on April 25, 2024: member

@setavenger paging @josibake.

setavenger commented at 2:16 pm on April 25, 2024: none

I don’t think extracting the public keys is the issue as they are pretty standard. So I think it has to be the input_hash. I don’t know C++ but I suspect that the linked part does (txid, vout_as_int) and compares the txid and then the vouts but as integers. The transaction in question has 4 outpoints for the lowest txid. And the vouts are 136, 202, 204 and 383. As bytes (LE) they are 0x88000000, 0xca000000, 0xcc000000 and 0x7f010000. Lexicographically sorted: 0x7f010000<0x88000000 but sorted as int one gets 136 < 383.

I just checked and if I manually change the smallest outpoint from using 383 to 136 I get the tweak that you gave above 0323fefb4e4bf637d7819c2030996dbe88d97c6a2f5708841df0e65fafd32c5a0a and with 383 I get 030acf723f1e3bfb392ccbe0460e28be1ac13cce512c3cfe618cce839aac6212bb.

Sjors commented at 2:58 pm on April 25, 2024: member

Sounds like a bug on our end then. The BIP says:

outpoint (36 bytes): the COutPoint of an input (32-byte txid, least significant byte first || 4-byte vout, least significant byte first)

And there’s a footnote saying “Why are outpoints little-endian”.

https://github.com/bitcoin/bips/blob/57c89ae162b4dab971dc6061ba6acf7d676781ea/bip-0352.mediawiki#cite_ref-why_little_endian_8-0

We should probably have a BIP352 specific COutPoint sorter, and a test vector.

setavenger commented at 3:05 pm on April 25, 2024: none

We should probably have a BIP352 specific COutPoint sorter, and a test vector.

Yeah, I’m working on a test vector that incorporates this kind of scenario.

DrahtBot added the label Needs rebase on Apr 25, 2024

DrahtBot commented at 7:04 pm on April 25, 2024: contributor

🐙 This pull request conflicts with the target branch and needs rebase.

josibake commented at 10:18 am on April 26, 2024: member

@Sjors I’ve updated #28122 to sort outpoints lexicographically and added @setavenger ’s test case. This should fix the discrepancy between your two indexes. Thanks again for testing this! Really glad we caught this one.

Sjors force-pushed on Apr 26, 2024

Sjors commented at 12:14 pm on April 26, 2024: member

Indexes match! If there’s other implementations out there, we should do the same comparison. @setavenger are you sorting transactions by their position in the block? That seems like the most logical canonical ordering to encourage.

setavenger commented at 1:57 pm on April 26, 2024: none

Great to hear that, thanks for running the comparison! I currently don’t sort or actually the ordering gets messed up due to parallel processing. I will probably include the possibility to sort the tweaks in the array.

The cut-through tweak index stores the txid for every tweak, so sorting them might not be that necessary. The cut through index also includes metadata like a dust indicator.

josibake commented at 2:46 pm on April 26, 2024: member

Indexes match! If there’s other implementations out there, we should do the same comparison.

Only other one I’m aware of is the cakewallet electrs fork, but they are using rust-silentpayments, which I veried is doing the outpoint sorting correctly (and passes @setavenger ’s new test case)

Sjors commented at 3:29 pm on April 26, 2024: member

I’m not a Rust guru, but if someone can write a quick script to make rust-silentpayments spit out the tweaks per block, I can compare it with our index.

cc @cygnet3

cygnet3 commented at 1:49 pm on May 2, 2024: none

Quickly made a tool to get the tweaks for a block using block height or block hash here (using rust-silentpayments)

$ cargo run --release -- --blkheight 800000

0[
1    "02740690cbe477ffcd2cf84a7bcb65a41adce26436758b00a58f3901605c3376eb",
2    "03f8470afaceb6946f3a01a37ecade692e8370f5cc9b2d1ff2edb7a3b01e857a3e",
3    "..."
4]

bitcoin deleted a comment on May 4, 2024

Squashed 'src/secp256k1/' changes from 0270b14309..92f592023f

92f592023f ci: enable silentpayments module
8ddc4574c9 tests: add BIP-352 test vectors
8315abd830 silentpayments: add benchmark for `scan_outputs`
f3a9516ec8 silentpayments: add examples/silentpayments.c
7e11e7613b silentpayments: add recipient light client support
3321771b0e silentpayments: add recipient scanning routine
766567f099 silentpayments: add opaque data type `public_data`
8d0bb06ce7 silentpayments: add recipient label support
9c9bd057bc silentpayments: add sender routine
036e688fd0 silentpayments: implement output pubkey creation
1ffee123d6 silentpayments: implement shared secret creation
7a5683260c silentpayments: add sortable recipient struct
a8d6f4b8e1 doc: add module description for silentpayments
1121a4d376 build: add skeleton for new silentpayments (BIP352) module
7d2591ce12 Add secp256k1_pubkey_sort
da515074e3 Merge bitcoin-core/secp256k1#1058: Signed-digit multi-comb ecmult_gen algorithm
4c341f89ab Add changelog entry for SDMC
a043940253 Permit COMB_BITS < 256 for exhaustive tests
39b2f2a321 Add test case for ecmult_gen recoded = {-1,0,1}
644e86de9a Reintroduce projective blinding
07810d9abb Reduce side channels from single-bit reads
a0d32b597d Optimization: use Nx32 representation for recoded bits
e03dcc44b5 Make secp256k1_scalar_get_bits support 32-bit reads
5005abee60 Rename scalar_get_bits -> scalar_get_bits_limb32; return uint32_t
6247f485b6 Optimization: avoid unnecessary doublings in precomputation
15d0cca2a6 Optimization: first table lookup needs no point addition
7a33db35cd Optimization: move (2^COMB_BITS-1)/2 term into ctx->scalar_offset
ed2a056f3d Provide 3 configurations accessible through ./configure
5f7be9f6a5 Always generate tables for current (blocks,teeth) config
fde1dfcd8d Signed-digit multi-comb ecmult_gen algorithm
486518b350 Make exhaustive tests's scalar_inverse(&x,&x) work
ab45c3e089 Initial gej blinding -> final ge blinding
aa00a6b892 Introduce CEIL_DIV macro and use it
REVERT: 0270b14309 labels: actually set the label
REVERT: 3d08027789 ci: enable silentpayments module
REVERT: 85946762a5 tests: add BIP-352 test vectors
REVERT: bf349c2a08 silentpayments: add examples/silentpayments.c
REVERT: 9a7106e19c silentpayments: add recipient light client support
REVERT: f113564298 silentpayments: add recipient scanning routine
REVERT: 4fb8716f4f silentpayments: add opaque data type `public_data`
REVERT: 987d829e8f silentpayments: add recipient label support
REVERT: 14ca754578 silentpayments: add sender routine
REVERT: 9b965927da silentpayments: implement output pubkey creation
REVERT: a0fcc2c780 silentpayments: implement shared secret creation
REVERT: 13f203dacd silentpayments: add sortable recipient struct
REVERT: a9326bdd7a doc: add module description for silentpayments
REVERT: 15d3e71cc1 build: add skeleton for new silentpayments (BIP352) module
REVERT: cc7d18a8a8 extrakeys: add secp256k1_pubkey_sort

git-subtree-dir: src/secp256k1
git-subtree-split: 92f592023f3f4d6a66724772349fbdc4967ab50f

2bfd600177

Merge commit '2bfd6001775c258f81e48b84624286fb9d898800' into refresh-secp256k1 41cf626708

Sjors commented at 12:25 pm on May 4, 2024: member

Took two days, but everything matches! (and we have the same sort)

Best to run this script in a few parallel windows, because after block 790K or so it gets extremely slow.

0./compare.sh 709632 790000

0./compare.sh 790000 800000

etc

 0#!/bin/bash
 1# Taproot activation: 709632
 2set -e
 3TIP=`bitcoin-cli getblockcount`
 4for height in $(seq $1 $2); do
 5  if [ $height -eq $TIP ]; then
 6    echo "Reached the tip at $height"
 7    exit 0
 8  fi
 9  if [ $((height % 1000)) -eq 0 ]; then
10    echo "$height..."
11  fi
12  HASH=`bitcoin-cli getblockhash $height`
13  diff <(cargo run --release --quiet -- --blkheight $height | jq) <(bitcoin-cli getsilentpaymentblockdata $HASH | jq '.tweaks') || (echo "$height: $HASH"; exit 1)
14done

crypto: add NUMS_H const b7724e9523

Compare COutPoints lexicographically cb0b68e9ca

Model the bech32 charlimit as an Enum

Bech32(m) was defined with a 90 character limit so that certain
guarantees for error detection could be made for segwit addresses.
However, there is nothing about the encoding scheme itself that requires
a limit and in practice bech32(m) has been used without the 90 char
limit (e.g. lightning invoices).

Further, increasing the character limit doesn't do away with error
detection, it simply lessons the guarantees.

Model charlimit as an Enum, so that if a different address scheme is
using bech32(m), the character limit for that address scheme can be
used, rather than always using the 90 charlimit defined for segwit
addresses.

145978049e

conf: add ECDH,SILENTPAYMENTS secp256k1 modules 9fec70a2d4

Add "sp" HRP 41f426aa22

Add V0SilentPaymentDestination address type 622c7a98b9

common: add bip352.{h,cpp} secp256k1 module

Wrap the silentpayments module from libsecp256k1. This is placed in
common as it is intended to be used by:

  * RPCs: for parsing addresses
  * Wallet: for sending, receiving, spending silent payment outputs
  * Node: for creating silent payment indexes for light clients

5b06ccf1dc

wallet: disable sending to silent payment address

Have `IsValidDestination` return false for silent payment destinations
and set an error string when decoding a silent payment address.

This prevents anyone from sending to a silent payment address before
sending is implemented in the wallet, but also allows the functions to
be used in the unit testing famework.

e565824a47

tests: add BIP352 test vectors as unit tests

Use the test vectors to test sending and receiving. A few cases are not
covered here, namely anything that requires testing specific to the
wallet. For example:

* Taproot script path spending is not tested, as that is better tested in
  a wallets coin selection / signing logic
* Re-computing outputs during RBF is not tested, as that is better
  tested in a wallets RBF logic

The unit tests are written in such a way that adding new test cases is
as easy as updating the JSON file

246e2a24c5

Sjors force-pushed on May 10, 2024

Sjors commented at 9:19 am on May 10, 2024: member

Rebased (on #28122) and renamed to -bip352index (stored in indexes/bip352). Presumably a future version will have its own BIP number, and by default get a fresh index. By that time we can figure out if it’s better to expand the existing index or have two separate non-overlapping indexes.

The index functions GetSilentPaymentKeys and FindSilentPayment are kept generic, so we have the option of subclassing the index.

I kept the getsilentpaymentblockdata RPC name generic, but renamed to tweaks to bip352_tweaks in the response.

I also added a MaySilentPayment helper in 24d0105aa78f48c1536c9daaef9e2ea6e8327801. @josibake feel free to pull that into the base PR.

josibake commented at 9:33 am on May 10, 2024: member

Rebased (on #28122) and renamed to -bip352index (stored in indexes/bip352).

Nice! Worth mentioning that now the indexes can be built without needing the wallet to be compiled.

Sjors force-pushed on May 10, 2024

Sjors commented at 1:17 pm on May 10, 2024: member

Added some scaffolding code for an index with cut-through: -bip352ctindex

I’m not sure yet how to actually implement this. The benefit of cut-through is that it’s smaller on disk and results in less data to download for light clients. But I suspect it will involve a lot more disk I/O to generate and maintain it.

Having both options make it easier to compare these trade-offs. It also gives users the option to serve the bigger index for light clients that need to recover their transaction history.

Sjors force-pushed on May 10, 2024

Sjors commented at 2:43 pm on May 10, 2024: member

Comparison at height 831,370:

without cut-through: 2.2 GB
with cut-through: 1.4 GB

A 35% reduction, slightly less than the 40% @setavenger found two months ago.

Next step is to (occasionally) prune the cut-through index.

I suspect the easiest way to is just to build a new index in the background, shut both the new and old indexes down, delete the old index and move the new one in place, and then turn it on again.

Another approach would be to override its contents block by block. If LevelDB handles resulting fragmentation in a sane way, this approach seems better.

In either case, something needs to trigger this process. Could be a timer, a fixed block interval or it could be an RPC call.

josibake commented at 6:05 pm on May 10, 2024: member

and results in less data to download for light clients

This is really the crucial benefit: if the index does not implement cut-through, clients will download data that is not relevant to them.

Something that will also likely cut the index down by a lot is a “dust filter,” i.e. prune transactions that have zero unspent taproot outputs above the dust filter. Something like 1000 sats, to give it some padding? The motivation here is a light client wont be able to spend these outputs anyways as they are uneconomical to spend and this can help protect light clients from “dusting attacks.”

setavenger commented at 6:24 pm on May 10, 2024: none

Something that will also likely cut the index down by a lot is a “dust filter,”

I have some preliminary numbers here. They are in the same ballpark as another analysis. Next step would be to see how this reduced UTXO set will actually affect the number of tweaks. Specifically, I’d like to find out to which degree we can actually reduce the set of tweaks based on how dust UTXOs are clustered per transaction.

Sjors force-pushed on May 10, 2024

Sjors commented at 6:54 pm on May 10, 2024: member

Alright, let’s see how it goes with dust limit 1000.

20ee76a1ef6eec43b5ceed123f6d5ad388185cd9 cuts it down to 300 MB. Assuming I implemented it correctly, that’s pretty nice!

in src/index/bip352.cpp:101 in 20ee76a1ef outdated

94+                        continue;
95+                    }
96+
97                     // Many new blocks may be processed while generating the index,
98                     // in between HaveCoin calls. This is not a problem, because
99                     // the cut-through index can safely have false positives.

Sjors commented at 6:56 pm on May 10, 2024:

Note that if we serve this via a BIP157 style p2p “filter”, the cut-through variant is going to be indeterministic. Same if people pick a different dust limit. This seems hard to avoid in any case, because the entire filter chain potentially changes with each new block that comes in.

josibake commented at 10:01 am on May 11, 2024:

Yeah, with cut-through and a dust filter, this starts to feel like not a good fit for a BIP157 style filter, only because we’d basically have to get rid of the filter chain and hashes to avoid making the server constantly recalculate. Another thing about dust filters is we could make it client query param, i.e. the server still keeps everything and then the client says “only send me stuff where the UTXOs have a value of >= X”. More storage space for the server, but clients then only download tweak data that they actually intend to spend. The nice thing about this model is the client can always query with a lower dust limit at any point to get everything else.

Got a few ideas on how we could make this work with a BIP157, but need to give it some more thought. It might also be case of trying to fit a square peg into a round hole, so maybe its better to start thinking from scratch on the protocol for serving the data.

(one obvious answer is we just keep the index in core and expose it via RPC, and then the exact p2p details are handled by something which uses core as a backend, e.g. electrumX).

Sjors commented at 7:15 am on May 13, 2024:

only send me stuff where the UTXOs have a value of >= X

So now we need an index and efficient >= filtering. (the latter isn’t too hard)

It might also be case of trying to fit a square peg into a round hole

For the index without cut-through I think BIP157 is fine. But to get the extra bandwidth savings of cut-through and a (custom) dust limit, it indeed seems we need something else. But ideally that shouldn’t block shipping something that works, even if it uses a bit more bandwidth that we’d like.

It’s worth considering at least adding the 546 sat dust limit to BIP352. Or at the very minimum exclude 0 sat outputs. I’ll re-run the normal index to see the effect.

josibake commented at 7:44 am on May 13, 2024:

For the index without cut-through I think BIP157 is fine

Agree.

It’s worth considering at least adding the 546 sat dust limit to BIP352

I’d argue these are all implementation details/optimizations. The BIP itself should be reserved for describing the protocol. There also might be use cases where where the value of the output is not the primary concern.

Sjors commented at 8:39 am on May 13, 2024:

I just checked two things:

LevelDB does not support indexes. It also does not support SQL queries.

If we were to store the amount (of the largest taproot output) in the index, any value filtering has to be done at the time we query the index. Worst case 9000 entries per block (a 1 input 1 output taproot transaction is 111 vbyte). That might be fine, you’d have to benchmark.

If it is a bottleneck, then we could refactor the base index to allow sqlite3 as a backend.

The only way to update an entry is to Delete(key) and Put(key). I currently use block hash as the key, so the ordering wouldn’t change. But what happens to the rest of the index file as you do this?

According to leveldb/doc/impl.md:

When the size of level L exceeds its limit, we compact it in a background thread.

Compactions drop overwritten values. They also drop deletion markers if there are no higher numbered levels that contain a file whose range overlaps the current key.

So if we (occasionally) override a block entry to update its cut-through, LevelDB should take care of defragging it (“compaction”). We should keep an eye on the resource usage there.

Sjors commented at 9:59 am on May 13, 2024:

Filtering < 546 dust makes no difference on the regular index size (unless I did something wrong in https://github.com/bitcoin/bitcoin/commit/411f035f0e09142311b6a829490f175582b07096)

setavenger commented at 10:04 am on May 13, 2024:

Filtering < 546 dust makes no difference on the regular index size (unless I did something wrong in 411f035)

That’s interesting you could compare with Blindbit Oracle. There is an exposed endpoint that filters based on a defined dust limit. There definitely is a difference at ~1000. I’m not sure whether I’ve tested <546 yet.

josibake commented at 10:10 am on May 13, 2024:

Not super surprising since this is a standardness limit, no? I don’t think you can broadcast transactions without the output amount being greater than the dust limit (unless it is an OP_RETURN).

setavenger commented at 10:19 am on May 13, 2024:

I did an analysis a while back which revealed that 10% of taproot UTXOs are <= 330. I’m not sure about the details but I believe it was the entire UTXO set collected by the indexer up to 834761. At the time it collected the entire set and not only SP-eligible UTXOs.

I guess it’s technically possible that either none of those 10% were in eligible transactions and/or they were combined with a bigger non-dust taproot UTXO.

Sjors commented at 12:44 pm on May 13, 2024:

Filtering < 546 dust

I should add that I was using IsDust() with DUST_RELAY_TX_FEE, which takes SegWit into account to decide the exact dust limit (apparently that’s 330). Note that dust uses <, not <=.

10% of taproot UTXOs

I looked at the largest output, not all outputs.

setavenger commented at 1:41 pm on May 13, 2024:

I should add that I was using IsDust() with DUST_RELAY_TX_FEE, which takes SegWit into account to decide the exact dust limit.

Ah I’m not sure how this should affect the results tbh. In my index I just looked at the UTXO value, no modifications.

I looked at the largest output, not all outputs.

Yeah but my current understanding is that in order to see no difference on the index size at all, all of those UTXOs must have been in combination with bigger UTXOs, not eligible at all or somehow affected by the DUST_RELAY_TX_FEE modification (not sure how that exactly works). How are the dust limits stored in the index? If they are based on the normal UTXO values we could do the same comparison as before #28241 (comment)

Sjors commented at 1:46 pm on May 13, 2024:

Dust limits are not stored in the index. https://github.com/bitcoin/bitcoin/pull/28241/commits/835dd4ba1ca4cc1abfd338e6bc38e25a116f5f8b stores the maximum output amount for each transaction. You can then use the RPC with any dust limit value you like.

setavenger commented at 1:50 pm on May 13, 2024:

My bad, that’s what I meant. So you can just say give me everything where at least one UTXO exists above that threshold value. That’s what my implementation does as well. Should be comparable then.

Sjors commented at 4:01 pm on May 13, 2024:

Exactly, with the caveat that you can use multiples of 16. How do I make the comparison in your script?

setavenger commented at 4:09 pm on May 13, 2024:

The tweaks endpoint with the dustLimit parameter. Any value should work I believe.

Sjors commented at 4:24 pm on May 13, 2024:

I’ll try that with 992 once I rebuilt the database. Is dustLimit >= 992 or > 992?

setavenger commented at 5:08 pm on May 13, 2024:

it’s >= 992. That index also implements cut-through btw.

Sjors commented at 6:44 pm on May 13, 2024:

Oh but I don’t want to use cut-through; that can only be compared when I generate the index at exactly the same height.

setavenger commented at 7:03 pm on May 13, 2024:

hmm that’s true, maybe I can quickly hack something together. It’s probably better to compare the base index anyways.

setavenger commented at 3:11 pm on May 14, 2024:

Took a bit longer than I expected. I’ve pushed to the dev branch. It seems to be working as intended. No cut-through and you can choose any value you like for a dustLimit. The request looks like this "localhost:8000/tweak-index/840000?dustLimit=546" (tweak-index then only returns tweaks where the highest value is >= 546)

Sjors commented at 10:57 am on May 22, 2024:

0curl -s localhost:8000/tweak-index/709632?dustLimit=992 | jq
1{
2  "error": "Server does not allow dustLimits"
3}

Do I need to add a setting or rebuild the index?

Sjors commented at 11:00 am on May 22, 2024:

Restarting with tweaks_full_with_dust_filter = 1

setavenger commented at 11:43 am on May 22, 2024:

Sorry, should have mentioned that before. Yes, that flag/setting is required.

Sjors commented at 12:53 pm on May 22, 2024:

Getting a mismatch on the first block:

0curl -s localhost:8000/tweak-index/709632?dustLimit=992 | jq

0[
1  "0282559668ac461407e39d78b0d604e19c88ec7d1b4eb1b07636a2fe10a00f9a74",
2  "0360b336c0121e9a1f64e5644a0519f2f6fd37ae28bc6558c604a63b1aac251a7c",
3  "03a1fbba688d7d1940910d2a40fdb08fa6c03e56ae3f1484941ae2b9451aa9672b",
4  "03e0b6b04bc4529c376dd8215dac77bb4d7696df34e097b28a7948686c2bc70b4c",
5  "02266e6da2790ffa1e47640eca18735281177b75e3435df731f41c42cc9646bdc0",
6  "03c4633c48038d5137eb4371707eed608ae6226c9aedf8d8ee1764b01d72561ac2"
7]

0bitcoin-cli getsilentpaymentblockdata  0000000000000000000687bca986194dc2c1f949318629b44bb54ec0a94d8244 992

0{
1  "bip352_tweaks": [
2    "03a1fbba688d7d1940910d2a40fdb08fa6c03e56ae3f1484941ae2b9451aa9672b",
3    "03e0b6b04bc4529c376dd8215dac77bb4d7696df34e097b28a7948686c2bc70b4c",
4    "02266e6da2790ffa1e47640eca18735281177b75e3435df731f41c42cc9646bdc0",
5    "03c4633c48038d5137eb4371707eed608ae6226c9aedf8d8ee1764b01d72561ac2"
6  ]
7}

Bitcoin Core excludes 0282559668ac461407e39d78b0d604e19c88ec7d1b4eb1b07636a2fe10a00f9a74 and 0360b336c0121e9a1f64e5644a0519f2f6fd37ae28bc6558c604a63b1aac251a7c. Without knowing which transactions those map too, I don’t know which side is wrong.

setavenger commented at 1:03 pm on May 22, 2024:

This branch should have a sorted index. We can now find the txid a lot easier, I think.

Sjors commented at 1:14 pm on May 22, 2024:

I added a fix for #28241 (review) and rebuilding the index now.

Sjors commented at 5:37 pm on May 22, 2024:

After my fix I found another inconsistency, at block 709,640 and dust limit 992 your index finds 3 while Bitcoin Core finds 2.

I manually looked up the eligible transactions:

Transaction (1) has one P2TR output of 897, which should get filtered. The second output of 1500 shouldn’t count, because it’s not taproot.

So I think the bug is on your end this time @setavenger :-)

setavenger commented at 9:00 am on May 31, 2024:

Wow completely missed the messages here. Yeah, was able to recreate the issue. I’ll figure out what the issue is and come back to you after I’ve got a fix for the problem.

setavenger commented at 10:09 am on May 31, 2024:

@Sjors it’s fixed https://github.com/setavenger/blindbit-oracle/pull/14 now. There was no taproot filter on the biggest utxo selection.

Sjors commented at 10:24 am on May 31, 2024:

Thanks, trying again…

Sjors commented at 1:52 pm on May 31, 2024:

Matches!

setavenger commented at 2:56 pm on May 31, 2024:

Cool! Would you mind sharing the current disk usage here https://github.com/setavenger/blindbit-oracle/issues/15 it’s been a while since I’ve done a full mainnet sync.

Sjors force-pushed on May 13, 2024

Sjors commented at 12:30 pm on May 13, 2024: member

I added the highest output amount for each transaction to the index, which the new dust argument for getsilentpaymentblockdata can use to filter.

The amount is right shifted by 4 and stored as a single byte. This lets us track a useful range (0 - 4080 sat) with only ~5% overhead.

in src/index/bip352.cpp:82 in 835dd4ba1c outdated

73@@ -74,6 +74,14 @@ bool BIP352Index::GetSilentPaymentKeys(std::vector<CTransactionRef> txs, CBlockU
74 
75         std::optional<CPubKey> tweaked_pk = BIP352::GetSerializedSilentPaymentsPublicData(tx->vin, coins);
76         if (tweaked_pk) {
77+            // Used to filter dust. To keep the index small we use only one byte
78+            // and measure in hexasats.
79+            uint8_t max_output_amount = UINT8_MAX; // 4080 sat
80+            for (const CTxOut& out : tx->vout) {
81+                // TODO: only consider Taproot outputs
82+                max_output_amount = std::min(out.nValue, CAmount(max_output_amount) << dust_shift) >> dust_shift;

Sjors commented at 1:00 pm on May 22, 2024:

835dd4ba1ca4cc1abfd338e6bc38e25a116f5f8b: oh this is wrong, it finds the lowest output amount, oops.

Sjors force-pushed on May 22, 2024

in src/index/bip352.cpp:82 in 337471e4ac outdated

73@@ -74,6 +74,14 @@ bool BIP352Index::GetSilentPaymentKeys(std::vector<CTransactionRef> txs, CBlockU
74 
75         std::optional<CPubKey> tweaked_pk = BIP352::GetSerializedSilentPaymentsPublicData(tx->vin, coins);
76         if (tweaked_pk) {
77+            // Used to filter dust. To keep the index small we use only one byte
78+            // and measure in hexasats.
79+            uint8_t max_output_amount = 0;
80+            for (const CTxOut& txout : tx->vout) {
81+                if (!BIP352::MaybeSilentPaymentOutput(txout)) continue;
82+                max_output_amount = std::max(txout.nValue, CAmount(max_output_amount) << dust_shift) >> dust_shift;

theStack commented at 1:17 pm on May 22, 2024:

Isn’t there some kind of special handling needed for large values that don’t fit into a byte after the shift (i.e. if txout.nValue >4080 sats)?

Sjors commented at 2:40 pm on May 22, 2024:

Thanks, fixed in 0f4ddfa43749266960a5d05693ed0cdc993328d6.

in src/common/bip352.cpp:80 in 3a34abecd4 outdated

72@@ -73,6 +73,24 @@ std::optional<PubKey> GetPubKeyFromInput(const CTxIn& txin, const CScript& spk)
73     return std::nullopt;
74 }
75 
76+
77+bool MaybeSilentPaymentOutput(const CTxOut& txout) {
78+    std::vector<std::vector<unsigned char>> solutions;
79+    return Solver(txout.scriptPubKey, solutions) == TxoutType::WITNESS_V1_TAPROOT;
80+}

theStack commented at 1:23 pm on May 22, 2024:

I’d suggest to introduce a fast CScript::IsPayToTaproot helper. It can be used both in MaybeSilentPayment and directly in the loop for finding the largest output amount (commit 337471e4acff439100488c16876d9f110ef04ef7), without the need to involve Solver:

 0diff --git a/src/script/script.cpp b/src/script/script.cpp
 1index 80e8d26bcf..d0e677aaa4 100644
 2--- a/src/script/script.cpp
 3+++ b/src/script/script.cpp
 4@@ -221,6 +221,14 @@ bool CScript::IsPayToWitnessScriptHash() const
 5             (*this)[1] == 0x20);
 6 }
 7 
 8+bool CScript::IsPayToTaproot() const
 9+{
10+    // Extra-fast test for pay-to-taproot CScripts:
11+    return (this->size() == 34 &&
12+            (*this)[0] == OP_1 &&
13+            (*this)[1] == 0x20);
14+}
15+
16 // A witness program is any valid CScript that consists of a 1-byte push opcode
17 // followed by a data push between 2 and 40 bytes.
18 bool CScript::IsWitnessProgram(int& version, std::vector<unsigned char>& program) const
19diff --git a/src/script/script.h b/src/script/script.h
20index 66d63fae89..283e8ff4de 100644
21--- a/src/script/script.h
22+++ b/src/script/script.h
23@@ -535,6 +535,7 @@ public:
24 
25     bool IsPayToScriptHash() const;
26     bool IsPayToWitnessScriptHash() const;
27+    bool IsPayToTaproot() const;
28     bool IsWitnessProgram(int& version, std::vector<unsigned char>& program) const;
29 
30     /** Called by IsStandardTx and P2SH/BIP62 VerifyScript (which makes it consensus-critical). */

Sjors commented at 2:40 pm on May 22, 2024:

Added in b2322fc476dc983b91492768d2054c2a3966afdd

Sjors force-pushed on May 22, 2024

Add CScript::IsPayToTaproot()

Co-authored-by: Sebastian Falbesoner <sebastian.falbesoner@gmail.com>

b2322fc476

Add MaybeSilentPayment helper e77a3ee30d

index: make FatalErrorf protected ff300df7c4

Clean up index logging

* don't log function name
* take into account that GetName() always ends with " index"

b22e2f81c8

index: add start_height to base index

This allows indexing to start from a specified start_height (default 0).
This is particularly useful for blockfilter indexes in that a new filter type could
be defined where the filter is only relevant after a certain block height (e.g.
a filter for only taproot scriptPubKeys).

Although this is currently not possible, start_height would also be needed if we wanted
to enable an index in pruned mode (e.g. having a txindex for recent blocks).

Co-Authored-by: Fabian Jahr <fjahr@protonmail.com>

45b79a657e

Add the silent payment index

Co-Authored-By: w0xlt <94266259+w0xlt@users.noreply.github.com>

7fbb31f231

rpc: add getsilentpaymentblockdata a0350f8d8a

Add cut-through index scaffold

Adds an index identical to -bip352index that can be enabled with -bip352ctindex.

Actual cut-through functionality is added in the next commit.

be38eeb267

Implement cut-through

Existing index are not updated, so in order to advantage of cut-through for new blocks, it needs to be occasionally deleted and rebuilt.

37efadad34

bip352: index highest output amount

This enables dust filtering. It has very little overhead.

0f4ddfa437

rpc: add dust arg to getsilentpaymentblockdata 473f11c967

Sjors force-pushed on May 22, 2024

Silent payment index (for light wallets and consistency check) #28241

Code Coverage

Reviews

Conflicts