This PR introduces waitNext(). It waits for either the tip to update or for fees at the top of the mempool to rise sufficiently. It then returns a new template, with which the caller can rinse and repeat.
On testnet3 and testnet4 the difficulty drops after 20 minutes, so the second ensures that a new template is returned in that case.
Alternative approach to #31003, suggested in #31109 (comment)
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31283.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | ryanofsky, ismaelsadeeq, vasild |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, this pull request conflicts with the following ones:
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.
993+ }
994+
995+ return block_template;
996+ }
997+
998+ const CScript m_script_pub_key;
script_pub_key around. Afaik it’s only used by tests and for solo CPU mining.
937@@ -938,9 +938,71 @@ class BlockTemplateImpl : public BlockTemplate
938 return chainman().ProcessNewBlock(block_ptr, /*force_processing=*/true, /*min_pow_checked=*/true, /*new_block=*/nullptr);
939 }
940
941+ std::unique_ptr<BlockTemplate> waitNext(CAmount fee_threshold, MillisecondsDouble timeout) override
942+ {
943+ if (timeout > std::chrono::years{100}) timeout = std::chrono::years{100}; // Upper bound to avoid UB in std::chrono
I dropped this line.
Additionally this PR does not make use of last_mempool_update, since it’s not very useful in a typical production setting where something in the mempool updates all the time.
I think the race condition mentioned there is not an issue here because we don’t pass in the current fees via argument, but instead derive them from the original template.
I did forget to update now at the end of the loop, which I fixed.
miner_tests to use the Mining interface. I plan to expand those tests to cover waitNext().
71-BlockAssembler MinerTestingSetup::AssemblerForTest(CTxMemPool& tx_mempool)
72-{
73- BlockAssembler::Options options;
74-
75- options.nBlockMaxWeight = MAX_BLOCK_WEIGHT;
76- options.blockMinFeeRate = blockMinFeeRate;
684 }
685- std::shared_ptr<const CBlock> shared_pblock = std::make_shared<const CBlock>(*pblock);
686- BOOST_CHECK(Assert(m_node.chainman)->ProcessNewBlock(shared_pblock, true, true, nullptr));
687- pblock->hashPrevBlock = pblock->GetHash();
688+ std::shared_ptr<const CBlock> shared_pblock = std::make_shared<const CBlock>(block);
689+ BOOST_REQUIRE(Assert(m_node.chainman)->ProcessNewBlock(shared_pblock, /*force_processing=*/true, /*min_pow_checked=*/true, nullptr));
miner.submitSolution() here.
🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/32946689392
Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:
Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
ProcessNewBlock and submitSolution via the Mining interface. The latter approach is much simpler, but both are used in production code, so should be covered.
waitNext().
In commit f9200f6c65b5a97a4842cc1ec34e688ec13bffcd
“Use createNewBlock via the interface instead of calling Chainman’s CreateNewBlock.”
Do you mean “Use createNewBlock via the interface instead of calling CreateNewBlock through the BlockAssembler directly”? There is no CreateNewBlock in the chainman.
987+ CAmount new_fees = 0;
988+ for (CAmount fee : block_template->m_block_template->vTxFees) {
989+ // Skip coinbase
990+ if (fee < 0) continue;
991+ new_fees += fee;
992+ if (new_fees >= current_fees + fee_threshold) return block_template;
fee_threshold of 0 combined with a timeout of 0 will immediately return a new template. This is useful for the Template Provider, because it needs to unconditionally send new templates to all connected clients if fees increased sufficiently for one its connected clients. (at least until cluster mempool makes the fee calculation trivially cheap)
re: #31283 (review)
In commit “Add waitNext() to BlockTemplate interface” (956b449a51223cfa620d6c76d7ec37db5de86c3b)
A
fee_thresholdof 0 combined with atimeoutof 0 will immediately return a new template. This is useful for the Template Provider, because it needs to unconditionally send new templates to all connected clients if fees increased sufficiently for one its connected clients. (at least until cluster mempool makes the fee calculation trivially cheap)
I don’t think I understand this special case. Wouldn’t it be easier for callers to just call createNewBlock() if they need a new template unconditionally? Dropping this would allow getting rid of the !tip_changed && timeout == MillisecondsDouble(0) && fee_threshold == 0 code which complicates things here.
Rebased after #31288 and #31122.
This PR is now based on #31325 since that has enough support. Keeping this draft until that’s merged (or dropped).
The test changes will probably cause a merge conflict with #31318, but it should be trivial. Ditto for the waitNext() implementation since that would no longer need to pump script_pub_key around.
I dropped the early return in waitNext() because wait_until should already take care of that. I also made the while loop use <= instead of < so this actually works: #31283 (review) Added a test for that.
958+ WAIT_LOCK(notifications().m_tip_block_mutex, lock);
959+ notifications().m_tip_block_cv.wait_until(lock, std::min(now + tick, deadline), [&]() EXCLUSIVE_LOCKS_REQUIRED(notifications().m_tip_block_mutex) {
960+ // Although C++17 allows safe comparison of optional<T> with
961+ // another T, we need to wait for m_tip_block to be set AND
962+ // for the value to be different than the current_tip value.
963+ tip_changed = notifications().m_tip_block && notifications().m_tip_block != m_block_template->block.hashPrevBlock;
If #31346 lands first, notifications().m_tip_block could be an Assert and the comment removed, see #31346 (comment)
Though I have to make sure there’s no race condition where you call createNewBlock() followed by waitNext() right at node startup, without first calling waitTipChanged(ZERO).
🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/34571636339
Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:
Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
Rebased on the latest #31581.
I added a commit that on testnet3 and testnet4 returns a new template after 20 minutes, to take advantage of the min difficulty rule.
Since miner_tests.cpp uses mainnet, I created testnet4_miner_tests.cpp to cover this behaviour in the unit tests. A functional test could be added later using the testnet4 blocks added by #31583. This could check if the difficulty is correct.
I also switched to using NodeClock::now() to be able to use mock time in tests.
🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/35210885796
Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:
Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
Rebased after #31581 landed, marking ready for review again.
If I need to retouch I’ll fix the typo mentioned in #31581 (review) and maybe add a helper to the test suggested here #31581 (review).
1001+ if (new_fees >= current_fees + fee_threshold) return block_template;
1002+ }
1003+ }
1004+
1005+ // Break out of while when using mock time
1006+ if (now == deadline) break;
In commit “Add waitNext() to BlockTemplate interface” (956b449a51223cfa620d6c76d7ec37db5de86c3b)
This seems hacky, Why not change now <= deadline to now < deadline at the top of the while loop, or alternately call setmocktime in tests with time + 1?
fee_threshold==0 && timeout==0 special case, I still need this. Added a comment to explain why (unit tests can’t easily move mock time during a blocking call).
unit tests can’t easily move mock time during a blocking call
Consider the following pattern to do that. I think it would be better to have that in the tests instead of this if (now == deadline) in the main (production) code.
0std::atomic_int now = 0;
1
2void waitNext(int timeout)
3{
4 const int deadline = now + timeout;
5 while (now < deadline) {
6 std::cout << "waitNext() now=" << now << "\n";
7 std::this_thread::sleep_for(500ms);
8 }
9}
10
11int main(int, char**)
12{
13 std::atomic_bool stop_advancing_mocktime{false};
14 std::thread advance_mocktime{[&stop_advancing_mocktime]() {
15 while (!stop_advancing_mocktime) {
16 ++now;
17 std::this_thread::sleep_for(500ms);
18 }
19 }};
20 waitNext(5);
21 stop_advancing_mocktime = true;
22 advance_mocktime.join();
23 return 0;
24}
I think it would be better to have that in the tests instead of this
if (now == deadline)in the main (production) code.
Do we actually think that behavior is bad to have in production code? It seems like it’s perfectly reasonable for production code to return either at the deadline or immediately after, and the choice is basically arbitrary for normal code but important for tests, so we are choosing a behavior that makes it more convenient to write tests and documenting that fact. This doesn’t seem l like a bad thing to me, and it might be preferable to introducing another thread and risking making the tests nondeterministic. Just my thoughts, no strong opinion though.
935@@ -936,9 +936,87 @@ class BlockTemplateImpl : public BlockTemplate
936 return chainman().ProcessNewBlock(block_ptr, /*force_processing=*/true, /*min_pow_checked=*/true, /*new_block=*/nullptr);
937 }
938
939+ std::unique_ptr<BlockTemplate> waitNext(CAmount fee_threshold, MillisecondsDouble timeout) override
In commit “Add waitNext() to BlockTemplate interface” (956b449a51223cfa620d6c76d7ec37db5de86c3b)
It might sense to move arguments to this function into a BlockWaitOptions struct similar to the existing BlockCreateOptions struct. Could make calling code clearer since it will specify need to specify arguments by name instead of passing two numbers, and provide more backwards source compatibility (require source code to change less if we want to drop existing arguments or add new ones in the future).
I dropped the special case for fee_threshold of 0 combined with a timeout of 0. I’ll adjust https://github.com/Sjors/bitcoin/pull/49.
Added a comment why the mock time workaround is still needed.
I also switched to using a new BlockWaitOptions struct.
Also rebased.
Code review ACK 6bf77e5a376804c0681897fa032537ba16e9a99a. Code looks great, but I still would like to spend more time reviewing tests in detail.
Main changes since last review were just adding options struct, dropping special case, adding comment explaining the now == deadline logic and other documentation. Code is pretty clear and simple now without the mempool GetTransactionsUpdated complexity, without the previous special case, and with the updated documentation.
55@@ -56,6 +56,16 @@ class BlockTemplate
56 * @returns if the block was processed, independent of block validity
57 */
58 virtual bool submitSolution(uint32_t version, uint32_t timestamp, uint32_t nonce, CTransactionRef coinbase) = 0;
59+
60+ /**
61+ * Waits for fees in the next block to rise, a new tip or the timeout.
62+ *
63+ * @param[in] options Control the timeout (default forever) and by how much total fees
64+ * for the next block should rise (default infinite).
In “Add waitNext() to BlockTemplate interface” a61fb2fd2d6b338c08c619e4605521a41bd3edd9
We already have the comments about the default in node types BlockWaitOptions docstring, writing it here is duplicating same information, when we update the default we have to update both places.
I think it will be better to just move the fee threshold description comment to node types BlockWaitOptions docstring and delete the duplicate here.
970+ * way to determine how much (approximate) fees for the next block increased.
971+ *
972+ * We'll also create a new template if the tip changed during the last tick.
973+ */
974+ if (options.fee_threshold < MAX_MONEY || tip_changed) {
975+ auto block_template{std::make_unique<BlockTemplateImpl>(m_assemble_options, BlockAssembler{chainman().ActiveChainstate(), context()->mempool.get(), m_assemble_options}.CreateNewBlock(), m_node)};
In “Add waitNext() to BlockTemplate interface” a61fb2fd2d6b338c08c619e4605521a41bd3edd9
I think we are not just creating a block template after ticks but on each iteration of this loop, which continues infinitely in the default case. Since we are holding cs_main continuously, wouldn’t this cause resource contention and delay other components of the node?
My concern is that, regardless of whether the mempool improves, we keep creating block templates.
Currently there is no reliable way to determine if the fee rate diagram of the mempool has increased, I think this will become easier after #30289. When RBF occurs, we can be certain that the fee rate diagram of the mempool has improved. We could even determine the fee additions after those RBFs and compare them with fee_threshold the pool want; if the threshold is exceeded, we should trigger a block template update.
Since that is not possible and #30289 is a WIP I think we should only build a block template after some time has passed. It seems like GBT waits for a minute maybe you could do the same?
re: #31283 (review)
In “Add waitNext() to BlockTemplate interface” a61fb2f
I think we are not just creating a block template after ticks but on each iteration of this loop, which continues infinitely in the default case. Since we are holding
cs_maincontinuously, wouldn’t this cause resource contention and delay other components of the node?
The m_tip_block_cv.wait_until(lock, std::min(now + tick, deadline)) call on line 957 is what is supposed to prevent this. Each time that line is reached the thread should sleep for one tick. It should only wake up before the tick if the tip changes or the call is timing out or the node is shutting down.
cs_main is only held briefly, after each 1 second wait, once per loop.
This PR currently makes 1 template per second to get the fees. Once we have cluster mempool, I’m hoping there will be a lighter weight method to see fees in the top 4 mega-weight units have risen. cc @sipa / @sdaftuar
One of the stratum v2 goals is to push new templates faster, so I’d rather keep the tick at 1 second unless it causes problems on slow hardware.
The m_tip_block_cv.wait_until(lock, std::min(now + tick, deadline)) call on line 957 is what is supposed to prevent this.
cs_main is only held briefly, after each 1 second wait, once per loop.
I missed this in first pass. Thanks for the clarifications my concern is addressed.
I imagine still there will be redundant work in doing this each second, we won’t be certain that much has changed in a second, but you could make the same argument for 30 seconds or 1 minute as well.
993+ for (CAmount fee : block_template->m_block_template->vTxFees) {
994+ // Skip coinbase
995+ if (fee < 0) continue;
996+ new_fees += fee;
997+ if (new_fees >= current_fees + options.fee_threshold) return block_template;
998+ }
nFees that also skips coinbase transaction, why aren’t we using that? could expose it and use it, this will prevent the iterating through all the block transactions to compute the total fees during each tick?
Good point. BlockAssembler::CreateNewBlock() uses nFees internally. It’s updated in AddToBlock. I could expose it via CBlockTemplate.
However this code will hopefully go away soon(tm) with Cluster Mempool, and I suspect it’s extremely fast (compared to block assembly). So it’s perhaps not worth refactoring. In any case it could be done in a followup without changing the interface.
63@@ -64,6 +64,9 @@ class BlockTemplate
64 * for the next block should rise (default infinite).
In “On testnet create min diff template after 20 mins” 6bf77e5a376804c0681897fa032537ba16e9a99a
nit: @Sjors should this commit message and comments be updated to reflect the actual change something like
“miner: update waitNext to return a new block if 20 minutes have elapsed since the tip”
I think the above discription is exactly what this commit does and does not enable the creation of the min diff template, it’s done internally by the block assembler?
Code Review 6bf77e5a376804c0681897fa032537ba16e9a99a
Just a question and a nit
61+ * Waits for fees in the next block to rise, a new tip or the timeout.
62+ *
63+ * @param[in] options Control the timeout (default forever) and by how much total fees
64+ * for the next block should rise (default infinite).
65+ *
66+ * @returns a new BlockTemplate or nullptr if the timeout occurs.
nullptr but an empty std::unique_ptr (which owns nullptr).
953+ while (now <= deadline) {
954+ bool tip_changed{false};
955+ {
956+ WAIT_LOCK(notifications().m_tip_block_mutex, lock);
957+ notifications().m_tip_block_cv.wait_until(lock, std::min(now + tick, deadline), [&]() EXCLUSIVE_LOCKS_REQUIRED(notifications().m_tip_block_mutex) {
958+ Assert(notifications().TipBlock());
TipBlock() returns an empty optional? Looks a bit too aggressive to me. TipBlock()’s doc says “Might be unset during an early shutdown”. Is it not better to return an empty unique_ptr from waitNext() if this happens?
I turned Assert into Assume. When comparing an std::optional<uint256> with a uint256 it will be considered unequal if the optional is empty. https://en.cppreference.com/w/cpp/utility/optional/operator_cmp (24)
This would trigger a spurious tip_changed. But since it can only happen during shutdown, it’s not a problem. And not fatal in any case.
944+ CAmount current_fees = -1;
945+
946+ // Alternate waiting for a new tip and checking if fees have risen.
947+ // The latter check is expensive so we only run it once per second.
948+ auto now{NodeClock::now()};
949+ const auto deadline = now + options.timeout;
options.timeout is MillisecondsDouble::max() it looks odd to add to it. This seems to work because of the underlying double type which ends up being inf and adding to inf results in inf. But if that is changed to an integer type this will break (overflow). Maybe at least comment on BlockWaitOptions::timeout that it must be of double type because of this, or to have BlockWaitOptions::timeout be of type std::optional with an empty optional meaning “no timeout”. Then deadline would be optional as well.
But if that is changed to an integer type this will break (overflow).
I don’t think there is anything unusual about that. If you change a floating point variable to an integer, you should expect code using that variable to need to be updated to handle overflows correctly, because integer and floating point numbers overflow differently. This code could work correctly if it were switched to an integer. It would just need to use saturating integer operations like the ones in util/overflow.h.
I also think an nullopt value is more semantically ambiguous. Specifying nullopt or “no timeout” could mean either return right away or wait forever. But specifying 1.7 x 10^308 seconds more clearly corresponds to waiting forever.
1014+ * unit tests would need to spawn a thread to achieve this.
1015+ * Making the while loop use now < deadline won't work either, because
1016+ * then there's no wait to test its internals, e.g. the 20 minute
1017+ * testnet exception.
1018+ */
1019+ if (now == deadline) break;
while (now < deadline) (instead of <=) and this should not exist. But then I do not understand what this means: “because then there’s no wait to test its internals, e.g. the 20 minute testnet exception”. Can you elaborate?
If the loop uses while (now < deadline) then it will never generate a template, so all the BOOST_REQUIRE(block_template) checks will fail.
So it has to use <=, but without the break here the test will freeze, because time doesn’t move.
You mean “never generate a template” when run from the tests, right? Why is that? The default timeout is inf so now < deadline should be true?
So it has to use <=, but without the break here the test will freeze, because time doesn’t move.
Right, but it is not too difficult to advance the time in unit tests.
Yes, when run from the tests.
It would require a second thread, because the main thread of the unit test is waiting on waitNext().
Ok, it will never generate a template because it will never exit from waitNext() (when run from unit tests with mocktime which never advances). “BOOST_REQUIRE(block_template) checks will fail” – they will never be reached in that case.
What about this (unit tests will use timeout=0):
0now = now()
1deadline = now + timeout;
2do {
3 stuff;
4 now = now();
5} while (now < deadline);
it will execute the body of the loop exactly once when given timeout=0 regardless of whether the clock advances or not.
I think it’s better explicitly mention the test issue:
RUN_COMMAND_DO_OTHER_THING_IN_NEW_THREAD_AFTER(command, delay)
(or 3. have an option to make mock time move)
then there’s no wait to test its internals
Should that be “no way”?
Will push fix if I need to change anything else.
Fixed
Consider the following. It avoids the hackish part and achieves the same purpose:
0diff --git i/src/node/interfaces.cpp w/src/node/interfaces.cpp
1index 9fe4984cd0..8022b47b5e 100644
2--- i/src/node/interfaces.cpp
3+++ w/src/node/interfaces.cpp
4@@ -960,13 +960,13 @@ public:
5 // could have been generated before a tip exists.
6 Assume(tip_block);
7 tip_changed = tip_block != m_block_template->block.hashPrevBlock;
8 return tip_changed;
9 };
10
11- while (now <= deadline) {
12+ do {
13 {
14 WAIT_LOCK(notifications().m_tip_block_mutex, lock);
15 // Note that wait_until() checks the predicate before waiting
16 notifications().m_tip_block_cv.wait_until(lock, std::min(now + tick, deadline), [&]() EXCLUSIVE_LOCKS_REQUIRED(notifications().m_tip_block_mutex) {
17 return check_tip_changed() || chainman().m_interrupt;
18 });
19@@ -1018,23 +1018,14 @@ public:
20 new_fees += fee;
21 Assume(options.fee_threshold != MAX_MONEY);
22 if (new_fees >= current_fees + options.fee_threshold) return block_template;
23 }
24 }
25
26- /**
27- * Break out of while when using mock time. While functional tests
28- * can call setmocktime to move the clock and exit this loop, a
29- * unit tests would need to spawn a thread to achieve this.
30- * Making the while loop use now < deadline won't work either, because
31- * then there's no wait to test its internals, e.g. the 20 minute
32- * testnet exception.
33- */
34- if (now == deadline) break;
35 now = NodeClock::now();
36- }
37+ } while (now < deadline);
38
39 return nullptr;
40 }
41
42 const BlockAssembler::Options m_assemble_options;
43
74+ /**
75+ * Wait until total fees in the new template exceed fees in the origal
76+ * template by at least this amount (in sats). The default is to ignore
77+ * fee increases and only wait for a tip change.
78+ */
79+ CAmount fee_threshold{MAX_MONEY};
std::optional with an empty optional meaning “no value” instead of the magic values MillisecondsDouble::max() and MAX_MONEY.
std::optional is not much friction. I’ll look into that.
IIRC I used magic values back when these were passed in as arguments, but in a struct using an
std::optionalis not much friction. I’ll look into that.
IMO, MAX_MONEY is a magic value that would be good to replace with nullopt, but MillisecondsDouble::max() is not a magic value and the code would be simpler if it were kept.
A magic value (imo) is a value that is handled specially and adds a special case, so replacing CAmount fee_threshold with std::optional` is good because the code actually has a contains a special case for not checking for any fee threshold, and so it is better to make that special case more explicit and directly controllable.
But the code does not contain any special case for MillisecondsDouble::max(), it is just treated correctly as a really large value, so changing that to use std::optional would only add a special case that wasn’t previously there and make the code more complicated and fragile. So it would be better to keep the way it is.
MillisecondsDouble::max(). Note here that we rely to have such a max value that when we add to it the result is the same max value (not overflow or cause unexpected things).
.capnp there’s no difference when making something an std::optional?
@ryanofsky do I understand correctly that for the
.capnpthere’s no difference when making something anstd::optional?
It depends on the type. In capnproto schemas primitive fields (numbers, bools, enums, unions) are not optional and are always present. If these fields are not assigned values they are just initialized to 0. So to represent a std::optional<T> value where T is a primitive you have to change the capnproto schema to be able to represent std::nullopt differently somehow, unless you are ok with std::nullopt being represented as 0.
Capnproto does allow non-primitive fields (data, text, struct, list fields) to be missing so you don’t need to change the capnproto schema to use std::optional for these. For these fields a std::nullopt field is just serialized with the field unset, and this is not ambiguous.
The difference between primitive/non-primitve fields is explained at https://capnproto.org/faq.html#how-do-i-make-a-field-optional and the faq recommends using a union with Void to represent optional primitive fields(*). Unfortunately though, I didn’t know about that trick and I don’t think libmultiprocess currently supports that, although it could. Another natural way to represent an optional value might just to use a List() type and only populate it with 0 or 1 elements.
But actually the way I’ve supported optional primitive fields in libmultiprocess so far has just been to accompany them with a boolean “has” field. So for example the Chain::getHeight method which returns std::optional<int> in c++ returns (result :Int32, hasResult :Bool) in capnproto. I know this works for parameters and returns values, though I’m not sure if it works for struct fields. You could just declare the struct to have a hasFeeThreshold: Bool value and I could fix this in libmultiprocess if it isn’t working.
Or if we think maybe List(Int64) or Union(Void, Int64) would be better ways to represent std::optional<Int64> fields we could use one of those representations and I can extend libmultiprocess to support them.
(*) I think the faq might be a little buggy, though, because I believe the Void field needs to come before the Int32 field in its example for Void to be the default value instead of 0.. EDIT: NVM faq is actually fine. I forgot the schema language allows setting default values so as long as the default value is void this is fine
MAX_MONEY as magic value to mean “no threshold” would be nice to avoid but is not a blocker for this PR. Accompanying boolean “has” field is fine too.
995+ for (CAmount fee : m_block_template->vTxFees) {
996+ // Skip coinbase
997+ if (fee < 0) continue;
998+ current_fees += fee;
999+ }
1000+ }
Looking at
isn’t this equivalent to just current_fees = m_block_template->vTxFees[0];?
That might work actually, with an extra minus. But it seems brittle, because the line pblocktemplate->vTxFees[0] = -nFees; doesn’t do anything afaik and might one day get deleted, causing this code to think fees are 0.
I just verified that deleting pblocktemplate->vTxFees[0] = -nFees; doesn’t break any test.
Additionally I expect this to go away in favor of something more efficient sooner rather than later, see #31283 (review)
pblocktemplate->vTxFees[0] = -nFees? Ie the test to fail if that line is removed?
I’d rather remove the line, it might have had a use before that no longer applies.
If we want to get fees via the CBLockTemplate then it should just get a field for that.
But I don’t think it’s useful to do any of that, because this (duplicate) code will likely go away soon: #31283 (review)
The real slowness is in constructing the CBLockTemplate, compared to that this loop should be negligible. And when we have a more efficient method to figure out the fee increase at the top of the mempool, we probably won’t use the CBLockTemplate struct at all.
re: #31283 (review)
This discussion is interesting but I don’t understand the line
at all. Why does it even make sense?
967+ LOCK(::cs_main);
968+
969+ // On test networks return a minimum difficulty block after 20 minutes
970+ if (!tip_changed && allow_min_difficulty) {
971+ const NodeClock::time_point tip_time{std::chrono::seconds{chainman().ActiveChain().Tip()->GetBlockTime()}};
972+ if (now > tip_time + std::chrono::seconds(20 * 60)) {
nit:
0 if (now > tip_time + 20min) {
964+
965+ // Must release m_tip_block_mutex before locking cs_main, to avoid deadlocks.
966+ LOCK(::cs_main);
967+ /**
968+ * The only way to determine if fees increased compared to the previous template,
969+ * is to generate a fresh template. Cluster Mempool may allow for a more efficient
This is not the case, or at least, this is not dependent on cluster mempool. EDIT: maybe it can actually be done much faster with cluster mempool, see below.
Cluster mempool will make the block-building algorithm faster (or, possibly, have the option of producing a better-quality one), but operationally nothing changes: you need to effectively run the block-building algorithm to know what fees can be gathered.
There are a number of options if we just want an approximate “how many fees” faster, but they could be implemented today just as easily:
I think eventually, the correct approach is to have a background thread that continuously creates blocks for the block weight(s) requested, and store the result in a variable that’s not under cs_main; the code here could then wait on it.
I was hoping we could simply have a variable that tracks the total fees for the top 1 vMB, which is constantly updated as things are inserted and removed from the mempool.
(or the ability to register multiple such variables which the code here can then monitor)
@sipa, I imagine that an RBF over any transaction in the last built block template be an indicator that the previously built template improve.
And since we would be using fee rate diagram for RBF’s after cluster mempool, it could also be an indicator? But higher mining score txs can also get it, so this is not that reliable.
Just discussed this with @ismaelsadeeq a bit more.
The hard part is not tracking improvements to the mempool (that is ultimately what all of cluster mempool is, making this explicit), but knowing which improvements affect the top 1 MvB of the mempool and to what extent. My thinking was that we can’t really maintain an index for that, as doing so would be less efficient than just recomputing when it’s needed.
But maybe that is not the case. We could keep track at all times of (a) the sum of the fees F and sizes S in all chunks that (fully) fit in the top 1 MvB of a block, and (b) the first chunk C that does not (fully) fit. Any time a chunk is added whose feerate exceeds C’s feerate, its fee/size is added to F & S. Any time such a chunk is removed, its fee is removed from F & S. Any time one wants to query the top block’s fee, C may need to be walked up or down, so to make S <= 1000000 and S+size(C) > 1000000, but that can be a pretty fast operation, which only scales with the number of chunks affected in between, not with the total number of chunks. When a new block comes in it may be faster to recompute F/S/C from scratch than trying to keep it updated though.
This would give a super fast way of estimating top block fee, though will ignore binpacking effects entirely.
This needs #30535, though.
952+ while (now <= deadline) {
953+ bool tip_changed{false};
954+ {
955+ WAIT_LOCK(notifications().m_tip_block_mutex, lock);
956+ notifications().m_tip_block_cv.wait_until(lock, std::min(now + tick, deadline), [&]() EXCLUSIVE_LOCKS_REQUIRED(notifications().m_tip_block_mutex) {
957+ Assume(notifications().TipBlock());
TipBlock() returns std::nullopt under non-error conditions (including shutdown)? If so, there should not be an Assume() here. Those are for conditions we believe are not possible if the code is correct.
The assume is correct, but the assert was a bit overzealous. waitNext() is a method on BlockTemplate. You can’t generate a template before there’s a tip, so it’s impossible for this fail.
It’s possible that the node is shut down when TipBlock() still returns std::nullopt, but in that scenario there can’t be a BlockTemplate. The value doesn’t get unset during shutdown.
but in that scenario there can’t be a
BlockTemplate…
What about
BlockTemplate is generatedTipBlock() returns an empty optional, triggering the Assume()Can’t happen?
m_tip_block.
@vasild it’s possible to start the node and quickly shut it down. If that happens then m_tip_block remains null.
If you call createNewBlock() during that time I’m not sure what happens. I think it’s best to add an extra guard there. The Template Provider calls waitTipChanged before calling createNewBlock(), but not every interface user might do that.
With that in place you can’t get here.
though I could include it here.
It turned into a bigger change than I expected, so better to keep it separate from this. I don’t think it’s blocking.
Or this, idea from discussion from another thread - return nullptr if TipBlock() returns no block:
0diff --git i/src/node/interfaces.cpp w/src/node/interfaces.cpp
1index 1eb66d81c0..9af5e65308 100644
2--- i/src/node/interfaces.cpp
3+++ w/src/node/interfaces.cpp
4@@ -947,35 +947,37 @@ public:
5 // The latter check is expensive so we only run it once per second.
6 auto now{NodeClock::now()};
7 const auto deadline = now + options.timeout;
8 const MillisecondsDouble tick{1000};
9 const bool allow_min_difficulty{chainman().GetParams().GetConsensus().fPowAllowMinDifficultyBlocks};
10
11+ std::optional<uint256> tip_hash;
12 bool tip_changed{false};
13 // Helper to check if the tip has changed, and also update tip_changed.
14- auto check_tip_changed = [this, &tip_changed]() EXCLUSIVE_LOCKS_REQUIRED(notifications().m_tip_block_mutex) {
15+ auto check_tip_changed = [this, &tip_hash, &tip_changed]() EXCLUSIVE_LOCKS_REQUIRED(notifications().m_tip_block_mutex) {
16 AssertLockHeld(notifications().m_tip_block_mutex);
17 if (tip_changed) return true;
18- const auto tip_block{notifications().TipBlock()};
19- // This is an instance method on BlockTemplate and no template
20- // could have been generated before a tip exists.
21- Assume(tip_block);
22- tip_changed = tip_block != m_block_template->block.hashPrevBlock;
23+ tip_hash = notifications().TipBlock();
24+ if (tip_hash.has_value()) {
25+ tip_changed = tip_hash.value() != m_block_template->block.hashPrevBlock;
26+ } else {
27+ tip_changed = true;
28+ }
29 return tip_changed;
30 };
31
32 do {
33 {
34 WAIT_LOCK(notifications().m_tip_block_mutex, lock);
35 // Note that wait_until() checks the predicate before waiting
36 notifications().m_tip_block_cv.wait_until(lock, std::min(now + tick, deadline), [&]() EXCLUSIVE_LOCKS_REQUIRED(notifications().m_tip_block_mutex) {
37 return check_tip_changed() || chainman().m_interrupt;
38 });
39 }
40
41- if (chainman().m_interrupt) return nullptr;
42+ if (chainman().m_interrupt || !tip_hash.has_value()) return nullptr;
43
44 // Must release m_tip_block_mutex before locking cs_main, to avoid deadlocks.
45 LOCK(::cs_main);
46
47 // On test networks return a minimum difficulty block after 20 minutes
48 if (!tip_changed && allow_min_difficulty) {
Does that look sensible?
it’s possible to start the node and quickly shut it down. If that happens then
m_tip_blockremains null. If you callcreateNewBlock()during that time I’m not sure what happens
The RPC is not yet serving requests when m_tip_block is assigned a value, but IPC is started before that. I put a long sleep just before assigning to m_tip_block but I am not sure how to call createNewBlock() from the IPC. Just reading the code, I think this assert in BlockAssembler::CreateNewBlock() will be triggered:
953+ bool tip_changed{false};
954+ {
955+ WAIT_LOCK(notifications().m_tip_block_mutex, lock);
956+ notifications().m_tip_block_cv.wait_until(lock, std::min(now + tick, deadline), [&]() EXCLUSIVE_LOCKS_REQUIRED(notifications().m_tip_block_mutex) {
957+ Assume(notifications().TipBlock());
958+ tip_changed = notifications().TipBlock() != m_block_template->block.hashPrevBlock;
(In case the Assume() above is correct, see my other comment).
It seems wasteful to call TipBlock() twice here. I’d suggest:
0auto tip_block = notifications().TipBlock();
1Assume(tip_block);
2tip_changed = m_block_template->block.hashPrevBlock;
3return tip_changed || chainman().m_interrupt;
39@@ -39,6 +40,11 @@ struct BlockCreateOptions $Proxy.wrap("node::BlockCreateOptions") {
40 coinbaseOutputMaxAdditionalSigops @2 :UInt64 $Proxy.name("coinbase_output_max_additional_sigops");
41 }
42
43+struct BlockWaitOptions $Proxy.wrap("node::BlockWaitOptions") {
In commit “miner: have waitNext return after 20 min on testnet” (22cf6cdb4190482b486752dae4e9dc3b44cfcb1e):
Note for a possible followup. I think this representation of the options struct should work fine for now as long as the client initializing the BlockWaitOptions struct is a c++ client using the libmultiprocess library.
However, this representation of the options struct might not be the most convenient for other clients, like a possible rust client, because by default these fields will both be set to 0 and interpreted by the server as 0, so the rust client will have to manually remember to set these fields to max values if they want default behavior.
This problem could be avoided by adding additional hasTimeout and hasFeeThreshold bool fields which will both default to false, and which should cause the c++ server code code to automatically leave the default c++ struct field values in place and not overwrite them with 0. Or other representations like the union(Void, T) representation mentioned #31283 (review) could also work. Any representation that results in default behavior when a capnproto client leaves these fields unset would work.
This can be a followup though, since default libmultiprocess c++ client will initialize both these fields from a source C++ struct which has the correct default values.
re: #31283 (review)
Actually never mind, there is a better solution here. I forgot capnproto does support setting default values, using syntax shown https://capnproto.org/language.html#structs, but for some reason I had thought it did not support them. It supports default values using a xor trick described https://capnproto.org/encoding.html#default-values).
So recommendation here could be to set default values in the capnproto struct to match default values in the c++ struct. But again this could be a followup and it only affects other potential clients like rust clients, not c++ client using libmultiprocess since they will just be serializing a c++ struct which already has the defaults set.
re: #31283 (review)
I’d really like this to be easy to use for a Rust client as early as possible. It seems better to revert to the magic value for now.
To make this easy for rust (or other) cilents using this interface you can assign these fields default values. Maybe define MAX_MONEY and MAX_TIMEOUT constants using syntax shown https://capnproto.org/language.html#constants and assign them as defaults using syntax shown https://capnproto.org/language.html#structs. These changes could be a followup though and shouldn’t be necessary for this PR.
951+
952+ while (now <= deadline) {
953+ bool tip_changed{false};
954+ {
955+ WAIT_LOCK(notifications().m_tip_block_mutex, lock);
956+ notifications().m_tip_block_cv.wait_until(lock, std::min(now + tick, deadline), [&]() EXCLUSIVE_LOCKS_REQUIRED(notifications().m_tip_block_mutex) {
notifications.TipBlock() has changed already, before the m_tip_block_mutex lock is grabbed? If so, we’d wait up to a full second before noticing?
wait_until already checks the predicate once before waiting. See #31283 (review)
975+ */
976+ if (options.fee_threshold || tip_changed) {
977+ auto block_template{std::make_unique<BlockTemplateImpl>(m_assemble_options, BlockAssembler{chainman().ActiveChainstate(), context()->mempool.get(), m_assemble_options}.CreateNewBlock(), m_node)};
978+
979+ // If the tip changed, return the new template regardless of its fees.
980+ if (tip_changed) {
cs_main is grabbed above? This could be easily detect by tip_changed || block_template->block.hashPrevBlock != m_block_template->block.hashPrevBlock here (the tip_changed || is perhaps not even necessary?).
m_tip_block_mutex and cs_main locks don’t jive well, so I limited the places where we actually check
MAX_MONEY magic value (with comment to explain why) and added a helper check_tip_changed() to make sure we check for a tip update before waiting.
949+ const auto deadline = now + options.timeout;
950+ const MillisecondsDouble tick{1000};
951+ const bool allow_min_difficulty{chainman().GetParams().GetConsensus().fPowAllowMinDifficultyBlocks};
952+
953+ bool tip_changed{false};
954+ // Helper to check if the tip has has changed, and also update tip_changed.
nit:
0 // Helper to check if the tip has changed, and also update tip_changed.
967+ {
968+ WAIT_LOCK(notifications().m_tip_block_mutex, lock);
969+ // First make sure the tip hasn't already changed:
970+ check_tip_changed() || notifications().m_tip_block_cv.wait_until(lock, std::min(now + tick, deadline), [&]() EXCLUSIVE_LOCKS_REQUIRED(notifications().m_tip_block_mutex) {
971+ return check_tip_changed() || chainman().m_interrupt;
972+ });
wait_until() will first call the predicate, so check_tip_changed() || is not needed. The above code is equivalent to:
0 notifications().m_tip_block_cv.wait_until(lock, std::min(now + tick, deadline), [&]() EXCLUSIVE_LOCKS_REQUIRED(notifications().m_tip_block_mutex) {
1 return check_tip_changed() || chainman().m_interrupt;
2 });
check_tip_changed() check before wait_until.
ACK 276ce2eea38865154017047f3761225c2b504cf6
Two things that would be nice to figure out before merge:
There could be a low-hanging fruit to avoid iterating over all transactions and use -nFee which should be significantly faster: #31283 (review)
Just to confirm that the Assume() can’t happen in normal circumstances: #31283 (review)
80+ * libmultiprocess maps missing values to 0.
81+ * https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937622086
82+ *
83+ * Use a magic value instead.
84+ */
85+ CAmount fee_threshold{MAX_MONEY};
In commit “Add waitNext() to BlockTemplate interface” (7f6f58bad8e56c9a25ffbc0d105bed37e9958fef)
This may all be true, but I don’t think it clearly explains the behavior when MAX_MONEY is specified. Would suggest a simpler comment:
fee_threshold sats higher than the current template, or unless the chain tip changes and the previous template is no longer valid. Determining whether fee_threshold is reached is expensive, so as an optimization, when fee_threshold is set to MAX_MONEY or higher, the implementation is able to be much more efficient, skipping expensive checks and only returning new templates when the chain tip changes.
re: #31283 (review)
In commit “Add waitNext() to BlockTemplate interface” (86449088b703d2110edaacbd6d1f1386916a3773)
I incorporated part of this comment. Note that some callers may just not be interested in fee changes, so I mentioned that as well.
Thanks, I do have two more suggestions:
MAX_MONEY.options.fee_threshold < MAX_MONEY check in the code and it would be less efficient internally but not have an observable difference from the callers perspective), it doesn’t make actually make sense to use std::optional here. Using std::optional would just make the interface more confusing and the implementation more complicated.955+ const auto deadline = now + options.timeout;
956+ const MillisecondsDouble tick{1000};
957+
958+ bool tip_changed{false};
959+ // Helper to check if the tip has changed, and also update tip_changed.
960+ auto check_tip_changed = [this, &tip_changed]() EXCLUSIVE_LOCKS_REQUIRED(notifications().m_tip_block_mutex) {
In commit “Add waitNext() to BlockTemplate interface” (7f6f58bad8e56c9a25ffbc0d105bed37e9958fef)
IMO it is confusing to define an extra lambda here that is only called in one place (inside another lambda). It would be less confusing to just inline check_tip_changed in the place where it is used, to remove a level of indirection and make the order code is written match the order that it executes. Would suggest
0--- a/src/node/interfaces.cpp
1+++ b/src/node/interfaces.cpp
2@@ -956,24 +956,19 @@ public:
3 const MillisecondsDouble tick{1000};
4
5 bool tip_changed{false};
6- // Helper to check if the tip has changed, and also update tip_changed.
7- auto check_tip_changed = [this, &tip_changed]() EXCLUSIVE_LOCKS_REQUIRED(notifications().m_tip_block_mutex) {
8- AssertLockHeld(notifications().m_tip_block_mutex);
9- if (tip_changed) return true;
10- const auto tip_block{notifications().TipBlock()};
11- // This is an instance method on BlockTemplate and no template
12- // could have been generated before a tip exists.
13- Assume(tip_block);
14- tip_changed = tip_block != m_block_template->block.hashPrevBlock;
15- return tip_changed;
16- };
17-
18 do {
19 {
20 WAIT_LOCK(notifications().m_tip_block_mutex, lock);
21 // Note that wait_until() checks the predicate before waiting
22 notifications().m_tip_block_cv.wait_until(lock, std::min(now + tick, deadline), [&]() EXCLUSIVE_LOCKS_REQUIRED(notifications().m_tip_block_mutex) {
23- return check_tip_changed() || chainman().m_interrupt;
24+ AssertLockHeld(notifications().m_tip_block_mutex);
25+ if (tip_changed || chainman().m_interrupt) return true;
26+ const auto tip_block{notifications().TipBlock()};
27+ // This is an instance method on BlockTemplate and no template
28+ // could have been generated before a tip exists.
29+ Assume(tip_block);
30+ tip_changed = tip_block != m_block_template->block.hashPrevBlock;
31+ return tip_changed;
32 });
33 }
34
check_tip_changed() lambda was introduced because that code had to be called in two places, then, following suggestions, it was changed to be called in just one place. I agree it would be better to remove it now, given that it is called in just one place.
988+ * (approximate) fees for the next block increased, perhaps more so after
989+ * Cluster Mempool.
990+ *
991+ * We'll also create a new template if the tip changed during the last tick.
992+ */
993+ if (options.fee_threshold != MAX_MONEY || tip_changed) {
In commit “Add waitNext() to BlockTemplate interface” (7f6f58bad8e56c9a25ffbc0d105bed37e9958fef)
IMO would be slightly better to write this as options.fee_threshold < MAX_MONEY || tip_changed, so MAX_MONEY is not a magic value, just an optimization cutoff for the point where we stop checking if fee_threshold has been reached because we know it is too high to be reached.
951+
952+ // Alternate waiting for a new tip and checking if fees have risen.
953+ // The latter check is expensive so we only run it once per second.
954+ auto now{NodeClock::now()};
955+ const auto deadline = now + options.timeout;
956+ const MillisecondsDouble tick{1000};
In commit “Add waitNext() to BlockTemplate interface” (7f6f58bad8e56c9a25ffbc0d105bed37e9958fef)
Any thoughts on whether it makes sense for this polling interval to be configurable? I could imagine a miner not really caring how expensive this check is and just wanting it to be as fast as possible?
A miner might naively set it so low that our p2p processing is impacted, potentially getting new blocks later and interfering with transaction relay.
Also, I hope to get rid of this soon(tm) if cluster mempool gives us a more efficient method. It’s nice if we don’t have to drop a configuration option again.
This is already 60x faster than the long poll behavior of getblocktemplate, which nobody complains about. And typical stratum (v1) jobs are refreshed every 30 seconds. https://b10c.me/blog/014-mining-pool-behavior-during-forks/
676@@ -672,8 +677,10 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity)
677 const int current_height{mining->getTip()->height};
678
679 // Simple block creation, nothing special yet:
680- block_template = mining->createNewBlock(options);
681- BOOST_REQUIRE(block_template);
682+ if (current_height % 2 == 0) {
683+ block_template = mining->createNewBlock(options);
684+ BOOST_REQUIRE(block_template);
685+ } // for odd heights the next template is created at the end of this loop
In commit “Add waitNext() to BlockTemplate interface” (7f6f58bad8e56c9a25ffbc0d105bed37e9958fef)
I found this comment pretty confusing and would suggest making it more specific like “// if current_height is odd, block_template will have already been set at the end of the previous loop.”
716@@ -710,7 +717,12 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity)
717 BOOST_REQUIRE_EQUAL(maybe_new_tip->GetBlockHash(), block.GetHash());
718 }
719 // This just adds coverage
In commit “Add waitNext() to BlockTemplate interface” (7f6f58bad8e56c9a25ffbc0d105bed37e9958fef)
The “// This just adds coverage” comment should be moved above the waitTipChanged call since it does not apply to the waitNext call, which is needed to set block_template for the next loop iteration.
962+ if (tip_changed) return true;
963+ const auto tip_block{notifications().TipBlock()};
964+ // This is an instance method on BlockTemplate and no template
965+ // could have been generated before a tip exists.
966+ Assume(tip_block);
967+ tip_changed = tip_block != m_block_template->block.hashPrevBlock;
In commit “Add waitNext() to BlockTemplate interface” (7f6f58bad8e56c9a25ffbc0d105bed37e9958fef)
Seem like it would be more correct (and more compact) to write this as:
tip_changed = Assume(tip_block) && tip_block != m_block_template->block.hashPrevBlock;
Since if tip_block is null the right thing to do is just wait for it to be non-null, not set tip_changed to true and try to return a new block template below.
Assume(tip_block) … if tip_block is null the right thing to do is just wait …
Hmm, if tip_block is null then Assume() will not wait but stop the program?
Taken
Assume(x) is x in production, so in the event of a bug tip_changed will be false. So the function will just wait for the deadline and return nothing.
Code review ACK 16b18f845313f25bbcdea122baa753b570add7d0. Just various suggested changes since last review.
(there seems to be a delay in Github processing my latest push)
Can confirm I see “Processing Updates” “Recent push is still being processed, and will show up in a bit” at the top of this PR.
877@@ -877,7 +878,7 @@ class ChainImpl : public Chain
878 class BlockTemplateImpl : public BlockTemplate
879 {
880 public:
881- explicit BlockTemplateImpl(std::unique_ptr<CBlockTemplate> block_template, NodeContext& node) : m_block_template(std::move(block_template)), m_node(node)
882+ explicit BlockTemplateImpl(BlockAssembler::Options assemble_options, std::unique_ptr<CBlockTemplate> block_template, NodeContext& node) : m_assemble_options(std::move(assemble_options)), m_block_template(std::move(block_template)), m_node(node)
clang-format doesn’t seem to touch this. However once I added some line breaks myself, it suddenly had an opinion. So I’ll commit that.
clang-format will not break long lines itself because src/.clang-format contains ColumnLimit: 0 (unlimited). However, if you break the line yourself it will do the alignment according to the other rulez.
I have a clang-format wrapper shell script that replaces ColumnLimit: 0 with ColumnLimit: 110 in src/.clang-format, runs the actual clang-format and then restores the file (so that I do not commit it accidentally).
960+ {
961+ WAIT_LOCK(notifications().m_tip_block_mutex, lock);
962+ // Note that wait_until() checks the predicate before waiting
963+ notifications().m_tip_block_cv.wait_until(lock, std::min(now + tick, deadline), [&]() EXCLUSIVE_LOCKS_REQUIRED(notifications().m_tip_block_mutex) {
964+ AssertLockHeld(notifications().m_tip_block_mutex);
965+ if (tip_changed) return true;
@Sjors , does tip_changed ever become true at line 956 and we return from there? I think it will always be false hence can be deleted.
When we first enter this scope, the predicate is executed before blocking. If tip_changed is updated to true, it is returned. And when the predicate returned value is true, we do not wait; instead, we proceed to build and return a block template.
In subsequent executions of the predicate tip_changed will be false and we will check if the tip has changed, if yes tip_changed is updated to true, and returned, causing the wait to end. We then proceed to build and return a block template.
re: https://github.com/bitcoin/bitcoin/pull/31283/files#r1960644463
Agree it looks like this line can be deleted. Also scope of tip_changed can be reduced so it is inside the do..while loop.
981+ * We determine if fees increased compared to the previous template by generating
982+ * a fresh template. There may be more efficient ways to determine how much
983+ * (approximate) fees for the next block increased, perhaps more so after
984+ * Cluster Mempool.
985+ *
986+ * We'll also create a new template if the tip changed during the last tick.
967+ const auto tip_block{notifications().TipBlock()};
968+ // We assume tip_block is set, because this is an instance
969+ // method on BlockTemplate and no template could have been
970+ // generated before a tip exists.
971+ tip_changed = Assume(tip_block) && tip_block != m_block_template->block.hashPrevBlock;
972+ return tip_changed;
This was return check_tip_changed() || chainman().m_interrupt; when the lambda check_tip_changed() was used. Now that the lambda is expanded here, the chainman().m_interrupt condition was lost. I think it should be:
0 return tip_changed || chainman().m_interrupt;
On testnet we need to create a min diff template after 20 min.
Rebased and addressed feedback. I also reformatted the BlockAssembler{ call to make it a bit less wide.
0--- a/src/node/interfaces.cpp
1+++ b/src/node/interfaces.cpp
2@@ -878,7 +878,11 @@ public:
3 class BlockTemplateImpl : public BlockTemplate
4 {
5 public:
6- explicit BlockTemplateImpl(BlockAssembler::Options assemble_options, std::unique_ptr<CBlockTemplate> block_template, NodeContext& node) : m_assemble_options(std::move(assemble_options)), m_block_template(std::move(block_template)), m_node(node)
7+ explicit BlockTemplateImpl(BlockAssembler::Options assemble_options,
8+ std::unique_ptr<CBlockTemplate> block_template,
9+ NodeContext& node) : m_assemble_options(std::move(assemble_options)),
10+ m_block_template(std::move(block_template)),
11+ m_node(node)
12 {
13 assert(m_block_template);
14 }
15@@ -956,24 +960,25 @@ public:
16 const MillisecondsDouble tick{1000};
17 const bool allow_min_difficulty{chainman().GetParams().GetConsensus().fPowAllowMinDifficultyBlocks};
18
19- bool tip_changed{false};
20 do {
21+ bool tip_changed{false};
22 {
23 WAIT_LOCK(notifications().m_tip_block_mutex, lock);
24 // Note that wait_until() checks the predicate before waiting
25 notifications().m_tip_block_cv.wait_until(lock, std::min(now + tick, deadline), [&]() EXCLUSIVE_LOCKS_REQUIRED(notifications().m_tip_block_mutex) {
26 AssertLockHeld(notifications().m_tip_block_mutex);
27- if (tip_changed) return true;
28 const auto tip_block{notifications().TipBlock()};
29 // We assume tip_block is set, because this is an instance
30 // method on BlockTemplate and no template could have been
31 // generated before a tip exists.
32 tip_changed = Assume(tip_block) && tip_block != m_block_template->block.hashPrevBlock;
33- return tip_changed;
34+ return tip_changed || chainman().m_interrupt;
35 });
36 }
37
38 if (chainman().m_interrupt) return nullptr;
39+ // At this point the tip changed, a full tick went by or we reached
40+ // the deadline.
41
42 // Must release m_tip_block_mutex before locking cs_main, to avoid deadlocks.
43 LOCK(::cs_main);
44@@ -992,15 +997,19 @@ public:
45 * (approximate) fees for the next block increased, perhaps more so after
46 * Cluster Mempool.
47 *
48- * We'll also create a new template if the tip changed during the last tick.
49+ * We'll also create a new template if the tip changed during this iteration.
50 */
51 if (options.fee_threshold < MAX_MONEY || tip_changed) {
52- auto block_template{std::make_unique<BlockTemplateImpl>(m_assemble_options, BlockAssembler{chainman().ActiveChainstate(), context()->mempool.get(), m_assemble_options}.CreateNewBlock(), m_node)};
53+ auto tmpl{std::make_unique<BlockTemplateImpl>(m_assemble_options,
54+ BlockAssembler{
55+ chainman().ActiveChainstate(),
56+ context()->mempool.get(),
57+ m_assemble_options}
58+ .CreateNewBlock(),
59+ m_node)};
60
61 // If the tip changed, return the new template regardless of its fees.
62- if (tip_changed) {
63- return block_template;
64- }
65+ if (tip_changed) return tmpl;
66
67 // Calculate the original template total fees if we haven't already
68 if (current_fees == -1) {
69@@ -1013,12 +1022,12 @@ public:
70 }
71
72 CAmount new_fees = 0;
73- for (CAmount fee : block_template->m_block_template->vTxFees) {
74+ for (CAmount fee : tmpl->m_block_template->vTxFees) {
75 // Skip coinbase
76 if (fee < 0) continue;
77 new_fees += fee;
78 Assume(options.fee_threshold != MAX_MONEY);
79- if (new_fees >= current_fees + options.fee_threshold) return block_template;
80+ if (new_fees >= current_fees + options.fee_threshold) return tmpl;
81 }
82 }
952+ // Delay calculating the current template fees, just in case a new block
953+ // comes in before the next tick.
954+ CAmount current_fees = -1;
955+
956+ // Alternate waiting for a new tip and checking if fees have risen.
957+ // The latter check is expensive so we only run it once per second.
m_interrupt check in the waitNext loop. Also made various code cleanups in both commits.
946@@ -942,9 +947,94 @@ class BlockTemplateImpl : public BlockTemplate
947 return chainman().ProcessNewBlock(block_ptr, /*force_processing=*/true, /*min_pow_checked=*/true, /*new_block=*/nullptr);
948 }
949
950+ std::unique_ptr<BlockTemplate> waitNext(BlockWaitOptions options) override
BlockTemplate object, as opposed to replacing the current (*this) one? As far as I can see the current one becomes useless, as its m_block_template will be outdated, so future calls to waitNext for it will return instantly anyway.
As far as I can see the current one becomes useless, as its
m_block_templatewill be outdated
This should not be the case. This returns a pointer to a new block template so miners are able to mine with the old one while calling this.
so future calls to
waitNextfor it will return instantly anyway.
They will need to call waitNext on the next template, but they can still call submitSolution on this one.
waitNext, but after it returns, isn’t it the case that the current BlockTemplate becomes useless?
Sure, while calling
waitNext, but after it returns, isn’t it the case that the currentBlockTemplatebecomes useless?
It’s not useless if submitSolution can still be called. This should be the case as long as the tip hasn’t changed and miners are still mining with this template. If the tip has changed or miners are no longer mining with this template, the client can discard it.
Indeed there can be quite a delay in when miners submit jobs. From the brief testing with ASIC machines I’ve seen, you might give it a new job and then several seconds later it’s still sending shares for the old job.
And it’s also possible that not every new template we create is actually deployed on every ASIC, as pool software might filter them (not the case at the moment afaik).