💬 hernanmarino commented on pull request "Debug Console implementation of generate method":
(https://github.com/bitcoin-core/gui/pull/692#issuecomment-1550418936)
As requested by @hebasto i splitted this PR in 2 commits :
ad5642ae91beb522b6ae806f28cb015a759d1d19 refactors the code to prepare for the new functionality implemented in the following commit, as well as for future console-only commands to be added.
411b1da407f78f2f973f90d48676ce1ae26734a7 adds the new "generate" command, as well as the updated "help generate"
(https://github.com/bitcoin-core/gui/pull/692#issuecomment-1550418936)
As requested by @hebasto i splitted this PR in 2 commits :
ad5642ae91beb522b6ae806f28cb015a759d1d19 refactors the code to prepare for the new functionality implemented in the following commit, as well as for future console-only commands to be added.
411b1da407f78f2f973f90d48676ce1ae26734a7 adds the new "generate" command, as well as the updated "help generate"
💬 ryanofsky commented on pull request "init: verify blocks data existence only once for all the indexers":
(https://github.com/bitcoin/bitcoin/pull/27607#issuecomment-1550419574)
> I can't see how the current approach could work, even if the problems discussed above were solved:
Good catch noticing Init() was not called! It doesn't seem like it should be that hard to fix, though. The PR was already moving most of the code out of Init(), anyway, so now a little more code needs to move. I didn't look very deeply but I would probably make Init() a public method and call it after constructing the index. Also stop calling Init() from Start() and move the RegisterValidation
...
(https://github.com/bitcoin/bitcoin/pull/27607#issuecomment-1550419574)
> I can't see how the current approach could work, even if the problems discussed above were solved:
Good catch noticing Init() was not called! It doesn't seem like it should be that hard to fix, though. The PR was already moving most of the code out of Init(), anyway, so now a little more code needs to move. I didn't look very deeply but I would probably make Init() a public method and call it after constructing the index. Also stop calling Init() from Start() and move the RegisterValidation
...
💬 furszy commented on pull request "init: verify blocks data existence only once for all the indexers":
(https://github.com/bitcoin/bitcoin/pull/27607#issuecomment-1550420156)
Thanks for the review @mzumsande.
Funny that I pushed a small update at the same time that you were commenting.
Have few more changes on the pipeline that will be pushing soon. e.g. the `hasDataFromTipDown` entire function can be written in two lines.. just need to re-purpose the `GetFirstStoredBlock` function a bit :).
> I think the necessary order would be to
>
> create all indexes and read their best block / other data from disk
determine the oldest block for all indexes
Do the p
...
(https://github.com/bitcoin/bitcoin/pull/27607#issuecomment-1550420156)
Thanks for the review @mzumsande.
Funny that I pushed a small update at the same time that you were commenting.
Have few more changes on the pipeline that will be pushing soon. e.g. the `hasDataFromTipDown` entire function can be written in two lines.. just need to re-purpose the `GetFirstStoredBlock` function a bit :).
> I think the necessary order would be to
>
> create all indexes and read their best block / other data from disk
determine the oldest block for all indexes
Do the p
...
💬 theuni commented on pull request "build: LLVM 15 & LLD based macOS toolchain":
(https://github.com/bitcoin/bitcoin/pull/21778#issuecomment-1550424047)
Hmm, Looks like [`fixup_chains` didn't go into lld until v16](https://github.com/llvm/llvm-project/commit/0d30e92f59589de44a185c53671b7fe2e83cd2ae). So I guess that would be our minimum version.
Which I suppose means that `fixup_chains` isn't actually being used in this PR. I'll check around in the logs and if I'm right about that I'll work up a test that this PR should fail as-is.
(https://github.com/bitcoin/bitcoin/pull/21778#issuecomment-1550424047)
Hmm, Looks like [`fixup_chains` didn't go into lld until v16](https://github.com/llvm/llvm-project/commit/0d30e92f59589de44a185c53671b7fe2e83cd2ae). So I guess that would be our minimum version.
Which I suppose means that `fixup_chains` isn't actually being used in this PR. I'll check around in the logs and if I'm right about that I'll work up a test that this PR should fail as-is.
💬 fjahr commented on pull request "assumeutxo: net_processing changes":
(https://github.com/bitcoin/bitcoin/pull/24008#issuecomment-1550427767)
> I wrote:
>
> > The first commit tends to confuses me
>
> Could use some thoughts on that comment (from anyone really).
@Sjors I found it hard to follow that message, to be honest :D Could you maybe try to formulate more clearly what is bothering you in particular and what questions you need to be answered? It might help if you reference the specific phases you are thinking of based on `doc/design/assumeutxo.md`. I think this is the most confusing part when discussing this.
(https://github.com/bitcoin/bitcoin/pull/24008#issuecomment-1550427767)
> I wrote:
>
> > The first commit tends to confuses me
>
> Could use some thoughts on that comment (from anyone really).
@Sjors I found it hard to follow that message, to be honest :D Could you maybe try to formulate more clearly what is bothering you in particular and what questions you need to be answered? It might help if you reference the specific phases you are thinking of based on `doc/design/assumeutxo.md`. I think this is the most confusing part when discussing this.
💬 hebasto commented on issue "Can't compile v24.0.1":
(https://github.com/bitcoin/bitcoin/issues/27680#issuecomment-1550432466)
> ### Steps to reproduce
>
> Setup Raspi4 with Raspi OS installed on a bootable SSD
> Install BDB 4.8
> Clone bitcoin repo 24.0.1
> configure
Try to run `./autogen.sh` before `./configure` as it is documented [here](https://github.com/bitcoin/bitcoin/blob/24.x/doc/build-unix.md#to-build).
(https://github.com/bitcoin/bitcoin/issues/27680#issuecomment-1550432466)
> ### Steps to reproduce
>
> Setup Raspi4 with Raspi OS installed on a bootable SSD
> Install BDB 4.8
> Clone bitcoin repo 24.0.1
> configure
Try to run `./autogen.sh` before `./configure` as it is documented [here](https://github.com/bitcoin/bitcoin/blob/24.x/doc/build-unix.md#to-build).
💬 LarryRuane commented on pull request "net: Allow inbound whitebind connections to more aggressively evict peers when slots are full":
(https://github.com/bitcoin/bitcoin/pull/27600#discussion_r1195757487)
Does RVO even apply here? I thought it applies to complex structures, a large vector being the canonical example, where we don't want to copy every element. But here we're just returning a `NodeId` which is a `int64_t`, wrapped by `std::optional`. I don't think there's any savings by moving instead of copying one of these.
(https://github.com/bitcoin/bitcoin/pull/27600#discussion_r1195757487)
Does RVO even apply here? I thought it applies to complex structures, a large vector being the canonical example, where we don't want to copy every element. But here we're just returning a `NodeId` which is a `int64_t`, wrapped by `std::optional`. I don't think there's any savings by moving instead of copying one of these.
💬 theuni commented on pull request "macOS: Bump minimum required runtime version and prepare for building with upstream LLVM":
(https://github.com/bitcoin/bitcoin/pull/27676#issuecomment-1550459263)
Pushed a test for `fixup_chains`, but I don't think it ever ran. @fanquake What sets `RUN_SECURITY_TESTS`?
(https://github.com/bitcoin/bitcoin/pull/27676#issuecomment-1550459263)
Pushed a test for `fixup_chains`, but I don't think it ever ran. @fanquake What sets `RUN_SECURITY_TESTS`?
⚠️ ddykeman1 opened an issue: "Mac osx 12.6.5 "
(https://github.com/bitcoin/bitcoin/issues/27681)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
I installed the new program it ran for 3 hrs and then I checked and said fatal error said to check logs there is no logs showing for this problem I uninstalled it and reinstalled a well as redownload it with the same out come no different
Did take a pic as it started and crashed
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
I installed the new program it ran for 3 hrs and then I checked and said fatal error said to check logs there is no logs showing for this problem I uninstalled it and reinstalled a well as redownload it with the same out come no different
Did take a pic as it started and crashed
![IMG_20230516_184337383](https://github.com/bitcoin/bitcoin/assets/52932653/7aa98bec-7da5-4bb8-a385-a0794e4
...
💬 theuni commented on pull request "ci: Use `macos-ventura-xcode:14.1` image for "macOS native" task":
(https://github.com/bitcoin/bitcoin/pull/26388#issuecomment-1550484640)
As [seen here](https://github.com/bitcoin/bitcoin/pull/27676#issuecomment-1550459263), this seems to have removed `RUN_SECURITY_TESTS`? Ping @hebasto
(https://github.com/bitcoin/bitcoin/pull/26388#issuecomment-1550484640)
As [seen here](https://github.com/bitcoin/bitcoin/pull/27676#issuecomment-1550459263), this seems to have removed `RUN_SECURITY_TESTS`? Ping @hebasto
💬 Xekyo commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#issuecomment-1550491250)
Super, thanks for the review, @theStack. As I already have a follow-up PR with #26152, since this PR has three ACKs now, and all the open comments are nits referring to the tests, I would like to include those changes as a new commit in the follow-up #26152.
What do you think, @glozow?
(https://github.com/bitcoin/bitcoin/pull/27021#issuecomment-1550491250)
Super, thanks for the review, @theStack. As I already have a follow-up PR with #26152, since this PR has three ACKs now, and all the open comments are nits referring to the tests, I would like to include those changes as a new commit in the follow-up #26152.
What do you think, @glozow?
💬 theStack commented on issue "Use muhash for assumeUTXO snapshot ":
(https://github.com/bitcoin/bitcoin/issues/27669#issuecomment-1550522541)
Had similar thoughts that using muhash instead would be nice in order to avoid the chain rollback for AssumeUTXO hash reviewers. Not sure if this is a huge problem, but one funny side-effect of this would be that for a chainstate with _n_ coins, there are in theory _n!_ different serializations possible that all would pass as valid (compared to just one for `hash_serialized_2`), since the order of coins doesn't matter for muhash.
> For good measure we should also modify `gettxoutsetinfo` so
...
(https://github.com/bitcoin/bitcoin/issues/27669#issuecomment-1550522541)
Had similar thoughts that using muhash instead would be nice in order to avoid the chain rollback for AssumeUTXO hash reviewers. Not sure if this is a huge problem, but one funny side-effect of this would be that for a chainstate with _n_ coins, there are in theory _n!_ different serializations possible that all would pass as valid (compared to just one for `hash_serialized_2`), since the order of coins doesn't matter for muhash.
> For good measure we should also modify `gettxoutsetinfo` so
...
💬 Xekyo commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#issuecomment-1550595711)
> ACK [6b605b9](https://github.com/bitcoin/bitcoin/commit/6b605b91c1faf2c7f7cc0c9d39b4fcfd66dc2965) modulo `miniminer_overlap` test.
>
> Not really blocking, I'm planning to go deeper later. And probably add some explanatory comments and code simplifications. I think that has a readability barrier that will be a maintenance issue moving forward.
Perhaps we can address that in #26152 as well
(https://github.com/bitcoin/bitcoin/pull/27021#issuecomment-1550595711)
> ACK [6b605b9](https://github.com/bitcoin/bitcoin/commit/6b605b91c1faf2c7f7cc0c9d39b4fcfd66dc2965) modulo `miniminer_overlap` test.
>
> Not really blocking, I'm planning to go deeper later. And probably add some explanatory comments and code simplifications. I think that has a readability barrier that will be a maintenance issue moving forward.
Perhaps we can address that in #26152 as well
💬 Xekyo commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#issuecomment-1550602703)
Rebased on #27021, need to rebase on master next, then will incorporate the follow-up nits from #27021
(https://github.com/bitcoin/bitcoin/pull/26152#issuecomment-1550602703)
Rebased on #27021, need to rebase on master next, then will incorporate the follow-up nits from #27021
🤔 ishaanam reviewed a pull request: "wallet: improve IBD sync time by skipping block scanning prior birth time"
(https://github.com/bitcoin/bitcoin/pull/27469#pullrequestreview-1429618571)
Approach ACK, this looks good so far but I haven't reviewed the first commit yet.
(https://github.com/bitcoin/bitcoin/pull/27469#pullrequestreview-1429618571)
Approach ACK, this looks good so far but I haven't reviewed the first commit yet.
💬 ishaanam commented on pull request "wallet: improve IBD sync time by skipping block scanning prior birth time":
(https://github.com/bitcoin/bitcoin/pull/27469#discussion_r1195808323)
nit:
```suggestion
/** Updates wallet birth time if 'new_birth_time' is lower than m_birth_time*/
```
(https://github.com/bitcoin/bitcoin/pull/27469#discussion_r1195808323)
nit:
```suggestion
/** Updates wallet birth time if 'new_birth_time' is lower than m_birth_time*/
```
💬 ishaanam commented on pull request "wallet: improve IBD sync time by skipping block scanning prior birth time":
(https://github.com/bitcoin/bitcoin/pull/27469#discussion_r1195809999)
Why is this check also done here if it is already done in `FirstKeyTimeChanged`?
(https://github.com/bitcoin/bitcoin/pull/27469#discussion_r1195809999)
Why is this check also done here if it is already done in `FirstKeyTimeChanged`?
💬 ajtowns commented on pull request "net_processing: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1195879972)
I mostly renamed it to make sure there'd be compiler errors anywhere that it didn't get updated, just in case some type conversion happened automatically.
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1195879972)
I mostly renamed it to make sure there'd be compiler errors anywhere that it didn't get updated, just in case some type conversion happened automatically.
💬 ajtowns commented on pull request "net_processing: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1195881028)
Yeah, this is broken as-is. My original idea was to add a timestamp to the `mapRelay` entries too, then it was to drop `mapRelay` and add `vExtraTxnForCompact`-relay, then I compromised by just doing it wrong...
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1195881028)
Yeah, this is broken as-is. My original idea was to add a timestamp to the `mapRelay` entries too, then it was to drop `mapRelay` and add `vExtraTxnForCompact`-relay, then I compromised by just doing it wrong...
💬 MarcoFalke commented on pull request "Descriptor unit tests and simplifications":
(https://github.com/bitcoin/bitcoin/pull/24361#issuecomment-1550798295)
> Drafted for now given it's (maybe) waiting on https://github.com/bitcoin/bitcoin/pull/26076.
That one was merged last week.
(https://github.com/bitcoin/bitcoin/pull/24361#issuecomment-1550798295)
> Drafted for now given it's (maybe) waiting on https://github.com/bitcoin/bitcoin/pull/26076.
That one was merged last week.