wallet: improve IBD sync time by skipping block scanning prior birth time

furszy commented at 7:32 pm on April 15, 2023: member

During initial block download, the node’s wallet(s) scans every arriving block looking for data that it owns. This process can be resource-intensive, as it involves sequentially scanning all transactions within each arriving block.

To avoid wasting processing power, we can skip blocks that occurred before the wallet’s creation time, since these blocks are guaranteed not to contain any relevant wallet data.

This has direct implications (an speed improvement) on the underlying blockchain synchronization process as well. The reason is that the validation interface queue is limited to 10 tasks per time. This means that no more than 10 blocks can be waiting for the wallet(s) to be processed while we are synchronizing the chain (activating the best chain to be more precise). Which can be a bottleneck if blocks arrive and are processed faster from the network than what they are processed by the wallet(s).

So, by skipping not relevant blocks in the wallet’s IBD scanning process, we will also improve the chain synchronization time.

DrahtBot commented at 7:32 pm on April 15, 2023: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	pinheadmz, ishaanam, achow101
Concept ACK	pablomartin4btc, MarcoFalke

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#27286 (wallet: Keep track of the wallet’s own transaction outputs in memory by achow101)
#26836 (wallet: finish addressbook encapsulation and simplify addressbook migration by furszy)
#26596 (wallet: Migrate legacy wallets to descriptor wallets without requiring BDB by achow101)
#26008 (wallet: cache IsMine scriptPubKeys to improve performance of descriptor wallets by achow101)
#25907 (wallet: rpc to add automatically generated descriptors by achow101)

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.

DrahtBot added the label Wallet on Apr 15, 2023

pinheadmz commented at 3:05 am on April 16, 2023: member

concept ACK, seems like a great idea.

in src/interfaces/chain.h:86 in 4698614908 outdated

82@@ -83,6 +83,7 @@ struct BlockInfo {
83     const uint256& hash;
84     const uint256* prev_hash = nullptr;
85     int height = -1;
86+    int64_t time = 0;

maflcko commented at 2:45 pm on April 17, 2023:

Any reason to use int64_t, when the block time is denoted in a type-safe NodeSeconds?

Edit: I guess all you need to do is switch from ...::GetBlockTime() to ...::Time()?

furszy commented at 1:43 pm on April 18, 2023:

Mainly to be consistent with the chain interface, wallet and spkm sources. Timestamps are represented as int64_t there. E.g. the FoundBlock class, findFirstBlockWithTimeAndHeight method, the CWallet::m_best_block_time member, first key time, descriptor creation time etc.

I think that is clear that block times are always in seconds, so the use of an arithmetic type shouldn’t be much of a worry but we could switch all of them to NodeSeconds too (probably in another PR that does it all at once?).

achow101 commented at 6:28 pm on April 19, 2023:

In 1a9a349a026af2865e5446091f1e072b7a845819 “wallet: skip block scan if block was created before wallet birthday”

CBlock already has a GetBlockTime(), so I don’t think it’s necessary to include the time as a separate field here.

However I’m not sure that we should be using the block’s timestamp for this time rather than MTP. With the block timestamp, we could ignore a more recent block that has a timestamp older than its parent which we wouldn’t ignore. That would be kinda bad.

furszy commented at 1:49 pm on April 20, 2023:

CBlock already has a GetBlockTime(), so I don’t think it’s necessary to include the time as a separate field here.

Yeah, that works here. The new field was because I initially implemented this on the filters-only sync branch. Where I have no blocks, only headers and filters.

However I’m not sure that we should be using the block’s timestamp for this time rather than MTP. With the block timestamp, we could ignore a more recent block that has a timestamp older than its parent which we wouldn’t ignore. That would be kinda bad.

Hmm, wasn’t the timestamp window created to extend the grace period enough so we can use the block time directly?. In other words, MTP moves the block time an hour back, and we have a grace period of two hours. If the two hours period are a concern, to not be forced to calculate the MTP for every block, what about doubling the grace period for IBD scanning?. Normally, it would just mean checking ~12 more blocks.

achow101 commented at 4:13 pm on April 20, 2023:

Hmm, wasn’t the timestamp window created to extend the grace period enough so we can use the block time directly?.

AFAIK it exists for accepting blocks with future timestamps, and we only use it in the wallet because it’s convenient.

But my comment isn’t really about the accuracy of the timestamp, but rather about the possibility of skipping a block after we have already started scanning. A block’s timestamp can be before its parent. The code as written would let us skip scanning a block even if we had scanned its parent. The reason to use MTP is because it can only move forward. The MTP of a block can never be less than the MTP of its parent, it can be equal, but that’s fine for us.

In RescanFromTime, we find blocks using a different metric, GetBlockTimeMax(). This is calculated as the maximum of the current block’s timestamp and it’s parent’s timestamp, so this too also can only move forward. If MTP is too slow to calculate, we could use GetBlockTimeMax() instead as it has the same property as always moving forwards.

furszy commented at 9:12 pm on April 20, 2023:

But my comment isn’t really about the accuracy of the timestamp, but rather about the possibility of skipping a block after we have already started scanning. A block’s timestamp can be before its parent. The code as written would let us skip scanning a block even if we had scanned its parent. The reason to use MTP is because it can only move forward. The MTP of a block can never be less than the MTP of its parent, it can be equal, but that’s fine for us.

Yeah, where I was going is that we really don’t care about those possible skipped blocks if the grace period is large enough. We know that the wallet will not find anything there, it’s just to ensure that all blocks created 2 hours before the wallet birth time will be scanned. In other words, The idea was to extend the window enough to not worry about unordered timestamps. E.g. going to the extreme, even if the grace period is one or two days, is much better than scanning the entire chain as we do now.

But.. point taken. The GetBlockTimeMax idea sounds nicer. It doesn’t require any extra calculation like MTP, nor grace period extension (which makes the wallet scan extra blocks). Just add another member into the BlockInfo struct and done.

in src/wallet/wallet.cpp:1793 in 4698614908 outdated

1783@@ -1777,6 +1784,14 @@ bool CWallet::ImportScriptPubKeys(const std::string& label, const std::set<CScri
1784     return true;
1785 }
1786 
1787+void CWallet::FirstKeyTimeChanged(const ScriptPubKeyMan* spkm, int64_t new_birth_time)
1788+{
1789+    int64_t birthtime = m_birth_time.load();
1790+    if (birthtime == BLANK_BIRTH_TIME || new_birth_time < birthtime) {
1791+        m_birth_time = new_birth_time;
1792+    }

pinheadmz commented at 3:19 pm on April 17, 2023:

why not a single-line if like you do below ?

furszy commented at 1:52 pm on April 18, 2023:

Because the other one is a return statement (the flow breaks there) while this one is a setter. Plus, when the line is too long, like this one, I think that braces help readability.

fanquake referenced this in commit 54e07a05b2 on Apr 17, 2023

in src/wallet/wallet.cpp:1441 in 4698614908 outdated

1433@@ -1434,6 +1434,13 @@ void CWallet::blockConnected(const interfaces::BlockInfo& block)
1434 
1435     m_last_block_processed_height = block.height;
1436     m_last_block_processed = block.hash;
1437+
1438+    // No need to scan block if block was created before the
1439+    // wallet birthday (as adjusted for block time variability)
1440+    const int64_t birth_time = m_birth_time.load();
1441+    if (birth_time == BLANK_BIRTH_TIME || block.time < birth_time - TIMESTAMP_WINDOW) return;

pinheadmz commented at 3:37 pm on April 17, 2023:

birthtime == BLANK_BIRTH_TIME – doesn’t this mean if a wallet has no known birthtime, it will never process any blocks?

furszy commented at 1:59 pm on April 18, 2023:

BLANK_BIRTH_TIME is for blank wallets. Blank wallets are wallets with no keys (no spkm exist). So it’s ok to skip blocks as the wallet isn’t going to find anything on them.

sidhujag referenced this in commit 2a7efd46df on Apr 17, 2023

in src/wallet/wallet.cpp:3606 in 4698614908 outdated

3603-        m_spk_managers[id] = std::move(spk_manager);
3604+        AddScriptPubKeyMan(std::move(spk_manager));
3605     } else {
3606         auto spk_manager = std::unique_ptr<ScriptPubKeyMan>(new DescriptorScriptPubKeyMan(*this, desc, m_keypool_size));
3607-        m_spk_managers[id] = std::move(spk_manager);
3608+        AddScriptPubKeyMan(std::move(spk_manager));

pinheadmz commented at 3:53 pm on April 17, 2023:

id is now an unused function argument LoadDescriptorScriptPubKeyMan(). Could clean that up? Actually I’m kinda curious why we ever needed to pass id if the value is always derivable from the descriptor itself.

furszy commented at 2:08 pm on April 18, 2023:

The id is the db lookup key for the descriptor, and also a way to detect db corruption.

We could clean it up from here, yeah. And probably, depending on its usage, we should also think about caching it.

furszy commented at 2:26 pm on April 18, 2023:

Ended up re-adding the id arg. So we don’t calculate the spkm id twice.

in src/wallet/wallet.cpp:3526 in 4698614908 outdated

3560+    const auto& spkm = m_spk_managers[spkm_man->GetID()] = std::move(spkm_man);
3561+
3562+    // Update birth time if needed
3563+    FirstKeyTimeChanged(spkm.get(), spkm->GetTimeFirstKey());
3564+}
3565+

pinheadmz commented at 3:55 pm on April 17, 2023:

I think adding this method is a good idea, and cleans up all the m_spk_managers[id] = ... which I like. I wonder if that could just be a separate commit since its just a nice refactor? And then add FIrstKeyTimeChanged() next

furszy commented at 2:26 pm on April 18, 2023:

sure, pushed.

pinheadmz commented at 4:14 pm on April 17, 2023: member

Did some general code review. I see we already have nCreateTime for descriptors and part of this PR seems to be just bringing that up to the wallet level, so that’s not a redundancy.

Also I don’t know if this is a problematic edge case or it could just be a good way to test but you could create a key and send BTC to it in a block, then a few blocks later import into a new descriptor and rescan. The wallet should miss the first transaction.

furszy force-pushed on Apr 18, 2023

furszy commented at 2:37 pm on April 18, 2023: member

Thanks for the review pinheadmz.

Also I don’t know if this is a problematic edge case or it could just be a good way to test but you could create a key and send BTC to it in a block, then a few blocks later import into a new descriptor and rescan. The wallet should miss the first transaction.

This PR is not modifying the rescan process. Only IBD. Guess that you provided a timestamp that is +2 hours ahead of the block time (network time variability window) or the “now” arg to skip rescan?

furszy force-pushed on Apr 18, 2023

pablomartin4btc commented at 5:23 pm on April 18, 2023: member

Concept ACK. Light code review, I’d to spend more time with it, which I’ll do soon. Good stuff!

furszy force-pushed on Apr 18, 2023

furszy force-pushed on Apr 19, 2023

in src/wallet/wallet.h:300 in 1a9a349a02 outdated

293@@ -294,6 +294,11 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
294     // Local time that the tip block was received. Used to schedule wallet rebroadcasts.
295     std::atomic<int64_t> m_best_block_time {0};
296 
297+    static const int64_t BLANK_BIRTH_TIME = -1;
298+    // First created key time. Used to skip blocks prior to this time.
299+    // 'BLANK_BIRTH_TIME' if wallet is blank.
300+    std::atomic<int64_t> m_birth_time{BLANK_BIRTH_TIME};

achow101 commented at 6:32 pm on April 19, 2023:

In 1a9a349a026af2865e5446091f1e072b7a845819 “wallet: skip block scan if block was created before wallet birthday”

m_birth_time could be initialized with std::numeric_limits<in64_t>::max() which would remove the need to compare against BLANK_BIRTH_TIME.

furszy commented at 2:00 pm on April 20, 2023:

pushed thanks

furszy force-pushed on Apr 20, 2023

furszy commented at 10:11 pm on April 20, 2023: member

Updated per feedback, thanks achow.

Changes:

Replaced BLANK_BIRTH_TIME constant by setting the m_birth_time to its max numeric value.
Tackled a block time variability concern by using the maximum time in the chain (a moving forward only timestamp) instead of the raw block time.
Extended the grace period to be twice as big as the one used in the rescan process. Even when this is not strictly needed, the rationale is to be a bit more conservative during IBD scanning because it’s an automatic process with no user interaction (unlike the rescan process that is usually triggered by an user action such an import).

in src/wallet/wallet.h:645 in 7be71a248a outdated

641@@ -638,6 +642,9 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
642     bool ImportPubKeys(const std::vector<CKeyID>& ordered_pubkeys, const std::map<CKeyID, CPubKey>& pubkey_map, const std::map<CKeyID, std::pair<CPubKey, KeyOriginInfo>>& key_origins, const bool add_keypool, const bool internal, const int64_t timestamp) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
643     bool ImportScriptPubKeys(const std::string& label, const std::set<CScript>& script_pub_keys, const bool have_solving_data, const bool apply_label, const int64_t timestamp) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
644 
645+    /** Updates wallet birth time if 'new_birth_time' is lower */

ishaanam commented at 0:33 am on May 17, 2023:

nit:

0    /** Updates wallet birth time if 'new_birth_time' is lower than m_birth_time*/

furszy commented at 9:26 pm on May 23, 2023:

pushed

in src/wallet/scriptpubkeyman.cpp:2743 in 7be71a248a outdated

2738     m_map_pubkeys.clear();
2739     m_map_script_pub_keys.clear();
2740     m_max_cached_index = -1;
2741     m_wallet_descriptor = descriptor;
2742+
2743+    if (signal_birth_time) NotifyFirstKeyTimeChanged(this, m_wallet_descriptor.creation_time);

ishaanam commented at 0:37 am on May 17, 2023:

Why is this check also done here if it is already done in FirstKeyTimeChanged?

furszy commented at 6:27 pm on May 23, 2023:

To save some extra processing, because they are performed at different levels. At the spkm level (here), we are at the lowest one. So, before emitting an event that will require to be processed by all the listeners (the wallets and/or other registered objects), we can just check if the updated descriptor timestamp is lower than the existent one.

but.. probably I’m thinking too far ahead and we could simplify this. Each spkm has only one listener atm.

ishaanam commented at 2:56 am on May 17, 2023: contributor

Approach ACK, this looks good so far but I haven’t reviewed the first commit yet.

furszy force-pushed on May 23, 2023

furszy commented at 9:26 pm on May 23, 2023: member

Updated per feedback, thanks @ishaanam.

achow101 commented at 9:40 pm on May 23, 2023: member

ACK 76396ca376188631ba46bd47b134881efcc6f755

DrahtBot added the label CI failed on May 23, 2023

ishaanam commented at 1:47 am on May 24, 2023: contributor

cr-ACK 76396ca376188631ba46bd47b134881efcc6f755

in src/test/util/mining.cpp:86 in f44da31c20 outdated

81@@ -82,7 +82,8 @@ std::shared_ptr<CBlock> PrepareBlock(const NodeContext& node, const CScript& coi
82             ->block);
83 
84     LOCK(cs_main);
85-    block->nTime = Assert(node.chainman)->ActiveChain().Tip()->GetMedianTimePast() + 1;
86+    const CBlockIndex* tip = Assert(node.chainman)->ActiveChain().Tip();
87+    block->nTime = tip->nHeight == 0 ? GetTime<std::chrono::seconds>().count() : tip->GetBlockTime() + 1;

maflcko commented at 5:56 am on May 24, 2023:

first commit: This would lead to issues in the rare case that more than 2 hours worth of blocks are produced, as they end up being in the future.

Is there a reason for the first commit?

maflcko commented at 5:57 am on May 24, 2023:

Also, it is a source of non-determinism, but I guess this is fine.

furszy commented at 5:48 pm on May 24, 2023:

Is there a reason for the first commit?

It is because this is the only place where we set the chain timestamps to be in the past. In the form of

0block_1_timestamp = genesis + 1
1block_2_timestamp = median(genesis, block_1) + 1
2block_3_timestamp = median(genesis, block_1, block_2) + 1

And in the wallet, we use the clock time to set the descriptors/keys creation time.

So, even when we create a wallet prior mining the chain, the wallet will always have a birth time after it. Which, with this PR, leads to the wallet skipping those blocks during sync. Which makes the wallet_balance.cpp benchmark fail in the balance assertion (the wallet has no balance because it skipped the blocks created prior its birth time).

But.. I just went deeper over it and.. it seems that the functions that create blocks in the past: generatetoaddress (which is not the RPC command) ~and PrepareBlock~ are only used in benchmarks: wallet_balance.cpp and block_assemble.cpp.. so we could re-work this stuff differently by cleaning some stuff.

The rest of the code uses BlockAssembler::CreateNewBlock, which sets the block time to the current, or mocked, clock time. So, no functional, unit or fuzz tests should be affected by this change.

furszy commented at 2:36 am on May 25, 2023:

Ended up pushing a simpler approach to not have to touch the fuzz tests for now (they are using PrepareBlock..). Focused on correcting wallet_balance.cpp only.

In the future, we could generate a fake chain instead of creating a “real” one. Similar to how it’s done in the wallet_create_tx.cpp benchmark.

maflcko commented at 5:54 am on May 25, 2023:

And in the wallet, we use the clock time to set the descriptors/keys creation time.

Ok, thanks for the background. However, I think the wallet uses mock time (NodeClock) for the times, so a much simpler fix would be just a single line patch to set the mock time to the genesis block time in the wallet bench that needs it?

furszy commented at 1:51 pm on May 25, 2023:

ha, loved it. I was so deep that ended up complicating things for no reason. Dropped the first commit and squashed the mock time change into the last one.

in src/test/fuzz/process_messages.cpp:42 in f44da31c20 outdated

38@@ -39,7 +39,9 @@ FUZZ_TARGET_INIT(process_messages, initialize_process_messages)
39 
40     ConnmanTestMsg& connman = *static_cast<ConnmanTestMsg*>(g_setup->m_node.connman.get());
41     TestChainState& chainstate = *static_cast<TestChainState*>(&g_setup->m_node.chainman->ActiveChainstate());
42-    SetMockTime(1610000000); // any time to successfully reset ibd

maflcko commented at 5:58 am on May 24, 2023:

first commit: Any reason this is changed?

furszy commented at 2:27 am on May 25, 2023:

not anymore. Re-worked the first commit so it only touches the wallet_balance.cpp bench file and nothing else.

maflcko commented at 6:02 am on May 24, 2023: member

left some comments/nits/questions. Feel free to ignore.

furszy force-pushed on May 24, 2023

furszy force-pushed on May 25, 2023

DrahtBot removed the label CI failed on May 25, 2023

refactor: single method to append new spkm to the wallet a082434d12

wallet: skip block scan if block was created before wallet birthday

To avoid wasting processing power, we can skip blocks that occurred
before the wallet's creation time,  since these blocks are guaranteed
not to contain any relevant wallet data.

This has direct implications (an speed improvement) on the underlying
blockchain synchronization process as well.

The reason is that the validation interface queue is limited to
10 tasks per time. This means that no more than 10 blocks can be
waiting for the wallet(s) to be processed while we are synchronizing
the chain (activating the best chain to be more precise).
Which can be a bottleneck if blocks arrive and are processed faster
from the network than what they are  processed by the wallet(s).

82bb7831fa

furszy force-pushed on May 25, 2023

maflcko approved

maflcko commented at 2:07 pm on May 25, 2023: member

Concept ACK. Haven’t reviewed the code.

in src/wallet/wallet.cpp:1442 in 82bb7831fa

1435@@ -1436,6 +1436,12 @@ void CWallet::blockConnected(const interfaces::BlockInfo& block)
1436 
1437     m_last_block_processed_height = block.height;
1438     m_last_block_processed = block.hash;
1439+
1440+    // No need to scan block if it was created before the wallet birthday.
1441+    // Uses chain max time and twice the grace period to adjust time for block time variability.
1442+    if (block.chain_time_max < m_birth_time.load() - (TIMESTAMP_WINDOW * 2)) return;

pinheadmz commented at 3:02 pm on May 25, 2023:

I’m curious if .load() is necessary for the atomic value? Is this the correct reason? https://stackoverflow.com/a/44288045/1653320

furszy commented at 3:20 pm on May 26, 2023:

It’s merely about consistency and readability.

The explicit load call tells the reader that this is an atomic variable, which could prevent some issues. E.g.

0if (block.chain_time_max < m_birth_time - (TIMESTAMP_WINDOW * 2)) return; 
1else (m_birth_time > something_esle) // do something else..

Here there are two atomic reads performed. And the m_birth_time could have changed in-between them.

pinheadmz commented at 3:17 pm on May 25, 2023: member

This PR is not modifying the rescan process. Only IBD.

sanity check: because we already skip old blocks during rescan?

https://github.com/bitcoin/bitcoin/pull/26679

furszy commented at 7:09 pm on May 25, 2023: member

sanity check: because we already skip old blocks during rescan?

#26679

Yes. Different entry points.

#26679 addressed the scanning of the existing chain during load time. And here, the emphasis shifts to the synchronization process when the wallet is active. When the node activates the best chain on each processed block and signal the “block connected” event.

pinheadmz commented at 11:14 pm on May 25, 2023: member

To review this PR I wrote a simple functional test: https://github.com/pinheadmz/bitcoin/commit/1f0f945aca3a4f5118e49b2d31318ddd4b19d906 which surprised me by failing. I added logging and comments to the test to share it because I don’t entirely understand what’s happening but it looks to me like, the wallet generates a combo(...) descriptor with a timestamp of 1 and therefore the wallet creation time is always 1 and so the new logic doesn’t actually skip any blocks. But if that’s the case, does that also mean #26679 doesn’t work either?

test output:

 02023-05-25T23:10:29.244000Z TestFramework (INFO): PRNG seed is: 8288012825723617302
 12023-05-25T23:10:29.245000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test__5pngcq0
 22023-05-25T23:10:29.886000Z TestFramework (INFO): Generate 10 blocks with 10-hour timestamp gaps
 32023-05-25T23:10:29.927000Z TestFramework (INFO): Initial blockchain sync the wallet's node
 4
 5not(500.00000000 == 250)
 6
 7Why does this descriptor have a timestamp of 1?
 8{'desc': 'combo(022d715e6f0d7c037dbccd3327f0b3bc3c76258a7e017e1a64902f76406c87dfda)#pesd53nq', 'timestamp': 1, 'active': False}
 9
102023-05-25T23:10:31.083000Z TestFramework (INFO): Stopping nodes
112023-05-25T23:10:31.306000Z TestFramework (WARNING): Not cleaning up dir /tmp/bitcoin_func_test__5pngcq0
122023-05-25T23:10:31.306000Z TestFramework (INFO): Tests successful

and then in bitcoin debug.log, this output from CWallet::blockConnected(): m_birth_time.load(): 1. Every time we run that function, the log says wallet birthday is 1

furszy commented at 0:27 am on May 26, 2023: member

To review this PR I wrote a simple functional test: https://github.com/pinheadmz/bitcoin/commit/1f0f945aca3a4f5118e49b2d31318ddd4b19d906 which surprised me by failing. I added logging and comments to the test to share it because I don’t entirely understand what’s happening but it looks to me like, the wallet generates a combo(…) descriptor with a timestamp of 1 and therefore the wallet creation time is always 1 and so the new logic doesn’t actually skip any blocks. But if that’s the case, does that also mean #26679 doesn’t work either?

The issue is on your test setup. The combo descriptor comes from the test framework default wallet creation process, not the inner wallet.

The framework, after creating the default wallet, imports one of the hardcoded private keys (link) used to receive the coinbases coins (link).

This means calling the test framework importprivkey, which is a proxy function with two possible outcomes:

For legacy wallet, call the RPC command importprivkey: which doesn’t receive a timestamp, so creation time is initialized to the minimum one.
For descriptors wallet, call the RPC command importdescriptors with timestamp=0. (which then is changed by the node internally to 1, because of the minimum supported timestamp)

The simplest solution for the test is to create the wallet by yourself, by calling createwallet, instead of using the default one initialized by the framework. And, remember to set the mock time before creating the wallet, so it uses the time established by you and not the clock time.

Thanks for testing :).

pinheadmz commented at 2:40 pm on May 26, 2023: member

@furszy awwwwwesome thank you for catching that. Ok, passing test is now at https://github.com/pinheadmz/bitcoin/commit/f1238700a1f3d3e88b7f20bc51f0088783d53595

pinheadmz approved

pinheadmz commented at 2:41 pm on May 26, 2023: member

ACK 82bb7831fa6052620998c7eef47e48ed594248a8

Reviewed code and tested locally, wrote a test. I am running this branch side-by-side with master on a VPS and will report who wins the race in a day or so ;-)

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA256
 2
 3ACK 82bb7831fa6052620998c7eef47e48ed594248a8
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmRwxNIACgkQ5+KYS2KJ
 7yTq5nhAA3dJDzRO4VmDUA8n9YO4RtpcQBHT/t9NOsoyNFszTZOQb47JqCTDSGBeZ
 8mwgdfTIMWDkIesi5lG81ATdGTvxeJjx/7OjN90x46Lj3bJuyVTJxrnrVtLuv/2tg
 9G9fnfXO2Ngod/daq9psvNhLfQLhW3CQNtsJ3x4NOJafKLOyxmd+dKxnXzJaKAicp
10ZlnIyUtArxLiDFXRmJRpmKAX2TRj49HooiY6gKlyyTtiOitB48AQdsh6hzaQ2O3q
11wyYzIwM8jEC3/hIbemPaoLphc6esevgXRCcEo3zv+VvnDdrtNBwXEKFtThkPniEq
125hF2zaVCB6cqlvjcFMnkZOVafxE9GWe7fJR7OPa0W2vS3YaSZD7zdhwTpK2KV0Cj
13HG21b2x5VFep6cQDl1Xqlktu21ExAWg2zOwESCnrJ7riRXtb3+Xe/eny90aeYhyX
140+ORbVy8AVyBYC1+nBjqpJA8JAoncm3xgNTpbgvX8XrflSaLcbhod5UAh0R+NE4c
15QGvGDa6ITO0+Cu/Yba4TwgOxzBmA/oXSxUecGR5eQi3KuKobVlfdLbKgjt1tfscs
16t414zb/cjcgpxVMXrrLZMMttg06I9YvSVUEcPCeU2YgiLgigTqunwsZ7KsOJM5ih
17HB4+y7/lIg+7kme6ZheZ221r7nWY6atR5piFe09AqLmKPBEviio=
18=VjDW
19-----END PGP SIGNATURE-----

pinheadmz’s public key is on keybase

DrahtBot requested review from achow101 on May 26, 2023

DrahtBot requested review from ishaanam on May 26, 2023

ishaanam commented at 10:09 pm on May 26, 2023: contributor

re-ACK 82bb7831fa6052620998c7eef47e48ed594248a8

DrahtBot removed review request from ishaanam on May 26, 2023

achow101 commented at 1:18 am on May 27, 2023: member

re-ACK 82bb7831fa6052620998c7eef47e48ed594248a8

DrahtBot removed review request from achow101 on May 27, 2023

achow101 merged this on May 27, 2023

achow101 closed this on May 27, 2023

furszy deleted the branch on May 27, 2023

sidhujag referenced this in commit 418ede0e7c on May 29, 2023

maflcko commented at 5:34 pm on May 30, 2023: member

Looks like the fuzz test needs an update?

082bb7831fa6052620998c7eef47e48ed594248a8 is the first bad commit

0echo 'AQAAAAEAAAA='|base64  --decode  > /tmp/a
1FUZZ=wallet_notifications ./src/test/fuzz/fuzz  /tmp/a

0fuzz: wallet/test/fuzz/notifications.cpp:175: void wallet::(anonymous namespace)::wallet_notifications_fuzz_target(FuzzBufferType): Assertion `total_amount == bal_a + bal_b' failed.
1==301525== ERROR: libFuzzer: deadly signal

furszy commented at 8:53 pm on May 30, 2023: member

Thanks Marco. Fixed in #27786. Would be nice if the CI alert us about this kind of stuff prior merging the PR. The base fuzzing corpus should have detected it right?.

maflcko commented at 5:58 am on May 31, 2023: member

The base fuzzing corpus should have detected it right?.

Yes, but the fuzz target is too slow right now, so the fuzz input directory is empty.

fanquake referenced this in commit 1b8b28d83b on May 31, 2023

sidhujag referenced this in commit 3802c64289 on May 31, 2023

achow101 referenced this in commit fbcf1029a7 on Oct 17, 2023

Frank-GER referenced this in commit 285ddfd388 on Oct 21, 2023

bitcoin locked this on May 30, 2024

wallet: improve IBD sync time by skipping block scanning prior birth time #27469

Reviews

Conflicts