💬 sdaftuar commented on pull request "validation: have LoadBlockIndex account for snapshot use":
(https://github.com/bitcoin/bitcoin/pull/23174#discussion_r1202821267)
The code here isn't comparing with the snapshot base height; it's just checking if a block index entry is below the `first_assumed_valid_height`, and only in that case does it get added to `setBlockIndexCandidates`. [I don't believe there's any other logic elsewhere that will do what you are describing, of adding entries below the snapshot base height to `setBlockIndexCandidates`?]
(https://github.com/bitcoin/bitcoin/pull/23174#discussion_r1202821267)
The code here isn't comparing with the snapshot base height; it's just checking if a block index entry is below the `first_assumed_valid_height`, and only in that case does it get added to `setBlockIndexCandidates`. [I don't believe there's any other logic elsewhere that will do what you are describing, of adding entries below the snapshot base height to `setBlockIndexCandidates`?]
💬 sdaftuar commented on pull request "validation: have LoadBlockIndex account for snapshot use":
(https://github.com/bitcoin/bitcoin/pull/23174#discussion_r1202825049)
More generally: I think that we don't need to worry at all about whether the assumed-valid bit is set when adding entries to `setBlockIndexCandidates`. We always validate a block when it gets connected, so the only invariant we need on `setBlockIndexCandidates` is whether they have enough work to be worth considering. (We used to have a hard requirement that we always HAVE_DATA for such entries, but after pruning we had to relax that a bit, as there are edge-case scenarios where you might HAVE_
...
(https://github.com/bitcoin/bitcoin/pull/23174#discussion_r1202825049)
More generally: I think that we don't need to worry at all about whether the assumed-valid bit is set when adding entries to `setBlockIndexCandidates`. We always validate a block when it gets connected, so the only invariant we need on `setBlockIndexCandidates` is whether they have enough work to be worth considering. (We used to have a hard requirement that we always HAVE_DATA for such entries, but after pruning we had to relax that a bit, as there are edge-case scenarios where you might HAVE_
...
💬 furszy commented on pull request "wallet: improve IBD sync time by skipping block scanning prior birth time":
(https://github.com/bitcoin/bitcoin/pull/27469#discussion_r1202827015)
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.
(https://github.com/bitcoin/bitcoin/pull/27469#discussion_r1202827015)
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.
💬 mzumsande commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1202830069)
Hmm, is the case where we are at `MAX_CMPCTBLOCKS_INFLIGHT_PER_BLOCK` but `BlockRequested()` is still called one something that can actually happen? Because in that case that caller would need to have previously checked that one of the existing requests is for the same nodeId, otherwise the limit would be breached.
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1202830069)
Hmm, is the case where we are at `MAX_CMPCTBLOCKS_INFLIGHT_PER_BLOCK` but `BlockRequested()` is still called one something that can actually happen? Because in that case that caller would need to have previously checked that one of the existing requests is for the same nodeId, otherwise the limit would be breached.
💬 jamesob commented on pull request "validation: have LoadBlockIndex account for snapshot use":
(https://github.com/bitcoin/bitcoin/pull/23174#discussion_r1202835874)
> The code here isn't comparing with the snapshot base height; it's just checking if a block index entry is below the first_assumed_valid_height
Oops, total misread on my part. You're right; `first_assumed_valid_height` will be the block after the tip of the background chain.
(https://github.com/bitcoin/bitcoin/pull/23174#discussion_r1202835874)
> The code here isn't comparing with the snapshot base height; it's just checking if a block index entry is below the first_assumed_valid_height
Oops, total misread on my part. You're right; `first_assumed_valid_height` will be the block after the tip of the background chain.
💬 ryanofsky commented on pull request "validation: have LoadBlockIndex account for snapshot use":
(https://github.com/bitcoin/bitcoin/pull/23174#discussion_r1202842452)
The logic here does seem confusing.
Code comment says background candidate set needs to exclude BLOCK_ASSUMED_VALID blocks because otherwise ActivateBestChain "would add assumed-valid blocks to the chain". But that seems like exactly what it should do? (after validating them)
Commit message says background candidate set needs to exclude blocks after the snapshot height that don't have BLOCK_ASSUMED_VALID, but depend on blocks that do have BLOCK_ASSUMED_VALID. That idea does make sense, but
...
(https://github.com/bitcoin/bitcoin/pull/23174#discussion_r1202842452)
The logic here does seem confusing.
Code comment says background candidate set needs to exclude BLOCK_ASSUMED_VALID blocks because otherwise ActivateBestChain "would add assumed-valid blocks to the chain". But that seems like exactly what it should do? (after validating them)
Commit message says background candidate set needs to exclude blocks after the snapshot height that don't have BLOCK_ASSUMED_VALID, but depend on blocks that do have BLOCK_ASSUMED_VALID. That idea does make sense, but
...
💬 instagibbs commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1202844909)
make sure to check the other call locations, but they aren't supposed to
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1202844909)
make sure to check the other call locations, but they aren't supposed to
💬 achow101 commented on pull request "test: fix intermittent issue in `feature_bip68_sequence`":
(https://github.com/bitcoin/bitcoin/pull/27177#issuecomment-1560024143)
ACK 272eb5561667482f8226bcf98eea00689dccefb8
(https://github.com/bitcoin/bitcoin/pull/27177#issuecomment-1560024143)
ACK 272eb5561667482f8226bcf98eea00689dccefb8
✅ achow101 closed an issue: "failure in feature_bip68_sequence.py"
(https://github.com/bitcoin/bitcoin/issues/27129)
(https://github.com/bitcoin/bitcoin/issues/27129)
🚀 achow101 merged a pull request: "test: fix intermittent issue in `feature_bip68_sequence`"
(https://github.com/bitcoin/bitcoin/pull/27177)
(https://github.com/bitcoin/bitcoin/pull/27177)
💬 jamesob commented on pull request "assumeutxo (2)":
(https://github.com/bitcoin/bitcoin/pull/27596#issuecomment-1560035238)
New bug, albeit pretty minor:
- [ ] if you restart the node after activating the snapshot but before connecting any blocks to the snapshot chainstate, `ReadBlockFromDisk` checks fail when calling `VerifyDB()` during chainstate initialization. I'm not sure when this would happen in practice, but it's an annoying edge case.
(https://github.com/bitcoin/bitcoin/pull/27596#issuecomment-1560035238)
New bug, albeit pretty minor:
- [ ] if you restart the node after activating the snapshot but before connecting any blocks to the snapshot chainstate, `ReadBlockFromDisk` checks fail when calling `VerifyDB()` during chainstate initialization. I'm not sure when this would happen in practice, but it's an annoying edge case.
💬 schjonhaug commented on issue "v25.0 testing":
(https://github.com/bitcoin/bitcoin/issues/27621#issuecomment-1560053023)
Looks like `>>` is missing after `"maxconnections=2"` in
```
$ cd $DATA_DIR && echo "signet=1" >> bitcoin.conf && echo "maxconnections=2" bitcoin.conf && cd ~
```
Also, it’s easier to copy the commands using the (hover) copy icon <img width="46" alt="image" src="https://github.com/bitcoin/bitcoin/assets/84225/ea868344-91f5-4e4b-b82c-5628f05a88bc"> on Github if you remove the `$` in front.
(https://github.com/bitcoin/bitcoin/issues/27621#issuecomment-1560053023)
Looks like `>>` is missing after `"maxconnections=2"` in
```
$ cd $DATA_DIR && echo "signet=1" >> bitcoin.conf && echo "maxconnections=2" bitcoin.conf && cd ~
```
Also, it’s easier to copy the commands using the (hover) copy icon <img width="46" alt="image" src="https://github.com/bitcoin/bitcoin/assets/84225/ea868344-91f5-4e4b-b82c-5628f05a88bc"> on Github if you remove the `$` in front.
💬 theuni commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1202937682)
Would it be possible to keep pointers/references to internal data out of this interface from the beginning? I'm not sure if that's a goal at this layer.
For blocktip for example, taking a quick look around, for the ui it looks like this eventually gets converted to:
`void(SynchronizationState, interfaces::BlockTip tip, double verification_progress)`
I wonder if we can do the same here?
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1202937682)
Would it be possible to keep pointers/references to internal data out of this interface from the beginning? I'm not sure if that's a goal at this layer.
For blocktip for example, taking a quick look around, for the ui it looks like this eventually gets converted to:
`void(SynchronizationState, interfaces::BlockTip tip, double verification_progress)`
I wonder if we can do the same here?
🤔 theuni reviewed a pull request: "kernel: Remove interface_ui, util/system from kernel library"
(https://github.com/bitcoin/bitcoin/pull/27636#pullrequestreview-1440486632)
From IRC:
> <TheCharlatan> Hey, how about moving the last two commits of #27636 (the only ones to touch the shutdown logic) to #27711? Then we can get #27636 merged and move ahead with #27576.
This is a good idea imo. I really don't want to slow this down, but I would benefit from a few extra days of catching up and testing the shutdown logic before reviewing/critiquing it in detail.
(https://github.com/bitcoin/bitcoin/pull/27636#pullrequestreview-1440486632)
From IRC:
> <TheCharlatan> Hey, how about moving the last two commits of #27636 (the only ones to touch the shutdown logic) to #27711? Then we can get #27636 merged and move ahead with #27576.
This is a good idea imo. I really don't want to slow this down, but I would benefit from a few extra days of catching up and testing the shutdown logic before reviewing/critiquing it in detail.
📝 fjahr opened a pull request: "init: Improve file descriptor limit handling"
(https://github.com/bitcoin/bitcoin/pull/27730)
I was playing with file descriptor limits to test another issue saw the following warning printed when I had configured a file descriptor limit of 150:
```
Warning: Reducing -maxconnections from 125 to -9, because of system limitations.
```
The warning could have been resolved with a one-line change but I found the surrounding code hard to reason about at first, so I added a few comments and reorganized the code a bit for better clarity.
Aside from the warning there is one further beh
...
(https://github.com/bitcoin/bitcoin/pull/27730)
I was playing with file descriptor limits to test another issue saw the following warning printed when I had configured a file descriptor limit of 150:
```
Warning: Reducing -maxconnections from 125 to -9, because of system limitations.
```
The warning could have been resolved with a one-line change but I found the surrounding code hard to reason about at first, so I added a few comments and reorganized the code a bit for better clarity.
Aside from the warning there is one further beh
...
💬 schjonhaug commented on issue "v25.0 testing":
(https://github.com/bitcoin/bitcoin/issues/27621#issuecomment-1560102597)
A comment on the following paragraphs:
> To create a non-witness transaction of 64 bytes or so, You can generate an OP_RETURN output with the following dummy data in hexadecimal format: 6a00000000.
> Get a new change address
> $ bcli -regtest getnewaddress
> You should create a transaction with two outputs: one that sends Bitcoin to the address you generated, and the other that is an OP_RETURN output with the hex value of 6a00000000
> $ bcli -regtest createrawtransaction '[{"txid":"04
...
(https://github.com/bitcoin/bitcoin/issues/27621#issuecomment-1560102597)
A comment on the following paragraphs:
> To create a non-witness transaction of 64 bytes or so, You can generate an OP_RETURN output with the following dummy data in hexadecimal format: 6a00000000.
> Get a new change address
> $ bcli -regtest getnewaddress
> You should create a transaction with two outputs: one that sends Bitcoin to the address you generated, and the other that is an OP_RETURN output with the hex value of 6a00000000
> $ bcli -regtest createrawtransaction '[{"txid":"04
...
💬 furszy commented on pull request "wallet: improve IBD sync time by skipping block scanning prior birth time":
(https://github.com/bitcoin/bitcoin/pull/27469#issuecomment-1560143021)
Updated per feedback, thanks @ishaanam.
(https://github.com/bitcoin/bitcoin/pull/27469#issuecomment-1560143021)
Updated per feedback, thanks @ishaanam.
💬 furszy commented on pull request "wallet: improve IBD sync time by skipping block scanning prior birth time":
(https://github.com/bitcoin/bitcoin/pull/27469#discussion_r1203034031)
pushed
(https://github.com/bitcoin/bitcoin/pull/27469#discussion_r1203034031)
pushed
💬 achow101 commented on pull request "Improve display address handling for external signer":
(https://github.com/bitcoin/bitcoin/pull/24313#discussion_r1203035210)
In 8218ce6b02b24eeef31f4e343ea777b7844907fc "wallet: return and display signer error"
Returning the error in an optional feels a bit wrong since it inverts are typical pattern. Rather than that the result is equivalent to `true`, we have to check that it's equivalent to `false`.
I think it would be better to use `util::Result<bool>` instead of an optional in order to preserve our typical error checking pattern.
(https://github.com/bitcoin/bitcoin/pull/24313#discussion_r1203035210)
In 8218ce6b02b24eeef31f4e343ea777b7844907fc "wallet: return and display signer error"
Returning the error in an optional feels a bit wrong since it inverts are typical pattern. Rather than that the result is equivalent to `true`, we have to check that it's equivalent to `false`.
I think it would be better to use `util::Result<bool>` instead of an optional in order to preserve our typical error checking pattern.
💬 achow101 commented on pull request "wallet: improve IBD sync time by skipping block scanning prior birth time":
(https://github.com/bitcoin/bitcoin/pull/27469#issuecomment-1560158824)
ACK 76396ca376188631ba46bd47b134881efcc6f755
(https://github.com/bitcoin/bitcoin/pull/27469#issuecomment-1560158824)
ACK 76396ca376188631ba46bd47b134881efcc6f755