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

Conflicts

Reviewers, this pull request conflicts with the following ones:

• Remove names from translatable strings #20404 (Remove names from translatable strings by hebasto)
• rpc: Remove duplicate name and argNames from CRPCCommand #20012 (rpc: Remove duplicate name and argNames from CRPCCommand by MarcoFalke)
• Flush dbcache early if system is under memory pressure #19873 (Flush dbcache early if system is under memory pressure by luke-jr)
• validation: UTXO snapshot activation #19806 (validation: UTXO snapshot activation by jamesob)
• Coinstats Index #19521 (Coinstats Index (without UTXO set hash) by fjahr)
• Allow maintaining the blockfilterindex when using prune #15946 (Allow maintaining the blockfilterindex when using prune by jonasschnelli)

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.

Thanks for all the reviews.

To answer some of @jimpo's questions:
2 & 4. I included part of the block hash so that in BlockDisconnected we ae sure to remove the entries in the index from this block only (that's where filter_by_value is used). The reason I chose to remove entries from the database is to prevent reading into a block file using an old CDiskTxPos that may no longer be a valid position. Otherwise in FindTxsByScript you could run into errors. You're right that this problem would be better handled by higher level methods.

I think that returning just the outpoint is a better idea than the current choice so I'll switch to that and try to incorporate all the other feedback here.

Can someone plz approve this?

Concept ACK

@marcinja will this allow other wallets like Electrum to utilize this feature (RPC credentials provided, of course) and avoid having to run ElecturmX (https://github.com/spesmilo/electrumx), EPS (https://github.com/chris-belcher/electrum-personal-server) or BWT (https://github.com/shesek/bwt) intermediary software?

@ecdsa @SomberNight @chris-belcher @shesek could you provide your input on how this feature might be useful (or not)?

The main issue AFAIU is that Electrum is using SHA256(scriptPubKey) while this PR is using MurmurHash3(scriptPubKey).
Also, ElectrumX & electrs are using RocksDB for the index storage - resulting in better performance and disk usage (compared to LevelDB).

@Talkless note that while some Electrum users run their own bitcoind, many do not. Electrum wants to support both use cases, and in fact the suspicion is that most users just use a public server. When using a public server, the client cannot use bitcoind RPC, hence in that case I don't see how the middleware (e.g. ElectrumX) could be avoided.

For the own bitcoind use case, maybe the client could have another optional mode of operation where it uses bitcoind RPC directly, which is I guess what you are asking about. For that, an address-index in bitcoind is the main thing missing indeed, however not the only one. For one, the electrum protocol (the client<->"middleware server" connection) has address subscriptions - the client gets a notification when a history of one its addresses changes. We are also planning on soon adding another method into the protocol that allows txoutpoint->spender_txid lookups (and notifications); I guess that could be implemented using the index in this PR albeit in a very inefficient way for heavily reused addresses.

IMHO there are multiple upsides for having this middleware setup for Electrum:

• for the project, it keeps the codebase simpler (again, we want to support users without own bitcoind)
• for the project, it also allows for more flexibility for implementing new functionality: just consider present PR here, we have needed such an index for 9 years but bitcoind did not have it or want it, so we could just implement it ourselves
• for the server operator, even if they don't want to open up the server for the public, they could share it with their friends and family. I don't think that's feasible with bitcoind RPC. I think this is a common use case.

Nevertheless if someone steps up and contributes patches, this kind of thing could be added.

The total size of the address index right now is 223GB

That sounds much larger than expected.
Even with the txoutpoint->spender_txid map I mentioned above, when using LevelDB, ElectrumX uses around 90 GiB of disk space.

The DB keys are structured as follows: <addr_id, key_type, outpoint>
The DB values are simply: <CDiskTxPos, CScript>

Instead of having both addr_id and CScript, why not just put a long hash, e.g. sha256(CScript) into the key?

Also, I expect most users of this index would also want txindex enabled. You might want to consider making address index dependent on txindex. Have you investigated how much space that would save? There would be no need to store CDiskTxPos.

Another trick that ElectrumX uses is that only the best chain is indexed. We have a tx_num->txid map as raw files on disk. A tx_num is the index of the transaction in the linear history of the blockchain, a 5-byte integer (so e.g. the genesis block coinbase tx has tx_num=0). This map uses around 17 GiB atm. With this, you can encode the txid part of the outpoint as 5 bytes instead of 32 bytes.

Concept ACK (might have already done that)

I'm concept -0 on this.

My primary objection is that I think it's a bad idea for any infrastructure to be built all that relies on having fully-indexed blockchain data available (this also applies to txindex, but we can't just remove support...).

However, it seems many people want something like this, and are going to use it anyway. The question is then whether it belongs in the bitcoin-core codebase. Alternative, and more performant presumably, like electrs exist already too, so it isn't exactly impossible to do this elsewhere.

Still, given that we now have the indexes infrastructure, it means that things like this are easy to add in a fairly modular way without invading consensus code. So if people really want this, fine.

Overall approach comment: I don't think MurmurHash should be used for anything new; there are strictly better hash functions available. I'd suggest SipHash if that's fast enough.

I'm also a little more negative on having this in Core than I previously was. After working in a few industrial contexts on wallet stuff, it's clear to me that an address index is really only required if you want to implement a block explorer or do chain analysis. For both of these applications, using something like electrs seems sufficient.

For personal wallet management, a full address index is not required. I think the origin of some confusion is that things like the Electrum Personal Server have become synonymous with this kind of usage, but in reality a full index is overkill when descriptor-specific rescans can be done once for a historical backfill and then per-block scanning can be done from there on out.

I want to point out that this is a nice implementation and good work by @marcinja, but I'm leaning slightly against the inclusion of such an index in Core at this point.

My primary objection is that I think it's a bad idea for any infrastructure to be built that relies on having fully-indexed blockchain data available.

I agree on this.

IMO the only use cases to ever use a full address index are:

• 1. Instant seed/xpriv backup recovery including spent history
• 2. Backend service for thousands of wallets
• 3. Debug/explore purposes

1 (instant backup recovery) could be solved with either scantxoutset (take a minute or two) or by an address-index for the utxo set only. But both would not restore the spent history.
A scalable non-enterprise solution to restore the spent history is using blockfilters. Scan through the filters and rescan only the relevant blocks (a matter of minutes), see #20664.

2 (a backend for thousands of wallets): out of scope for this project.

3 (explore purposes): I think this is a valid use case. Though adding this PR to Bitcoin Core will lead to many many projects using it in production increasing the traffic in this project and eventually steal time from existing contributors (rebase, maintenance, drag-along)

My main fear is that people are going to use this index (a full address index) to use it as an electrum(ish) backend for a handful of wallets.

With multiwallet, watch-only-wallets, PSBT, we have all tools to server multiple wallets in a scalable way for external applications.

I also think merging this as it is, would be in contradiction to the process- and repository-separation effort.

Therefore I'm ~0 (slightly towards NACK) to add this.
If this would be in another repository (still under bitcoin/*) and process separated, I would ACK it.

Hi all, thanks for the feedback and review. This was an enjoyable PR to work on and I learned a lot from all your comments.

I'm closing this PR because its size probably requires stronger support from contributors to get in. It also seems more clear now that all of the practical use-cases are covered by existing features and some lightweight alternatives (#20664) .

I also agree that it would be bad to incentivize using an address index to support an external electrum wallet, when it's not the intended use-case and would cause unnecessary burden on contributors and maintainers in this project, e.g. from users of those wallets wanting new features or updates.

@marcinja thank you and I hope to see more contributions of this quality from you.