π¬ achow101 commented on pull request "wallet: Keep track of the wallet's own transaction outputs in memory":
(https://github.com/bitcoin/bitcoin/pull/27286#discussion_r2091969986)
Done as suggested
(https://github.com/bitcoin/bitcoin/pull/27286#discussion_r2091969986)
Done as suggested
π¬ achow101 commented on pull request "wallet: Keep track of the wallet's own transaction outputs in memory":
(https://github.com/bitcoin/bitcoin/pull/27286#discussion_r2091971018)
As-is is more legible, and this area of code changes significantly in #27865 where having it like it is now makes those changes easier to read.
(https://github.com/bitcoin/bitcoin/pull/27286#discussion_r2091971018)
As-is is more legible, and this area of code changes significantly in #27865 where having it like it is now makes those changes easier to read.
π¬ hodlinator commented on pull request "(RFC) kernel: Replace leveldb-based BlockTreeDB with flat-file based store":
(https://github.com/bitcoin/bitcoin/pull/32427#issuecomment-2885100223)
### Re: One file per block
FWIW the idea of using one file per block gets me going too. :) It is slightly orthogonal but would be nice to avoid changing formats twice in short succession.
My bikeshed color: Since block hashes start with zeroes, maybe one could shard based off the last two bytes:
Genesis block ends up in something like:
`$DATADIR/blocks/e2/6f/000000000019d6689c085ae165831e934ff763ae46a2a6c172b3f1b60a8ce26f.dat`
Block [896819](https://mempool.space/block/000000000000000
...
(https://github.com/bitcoin/bitcoin/pull/32427#issuecomment-2885100223)
### Re: One file per block
FWIW the idea of using one file per block gets me going too. :) It is slightly orthogonal but would be nice to avoid changing formats twice in short succession.
My bikeshed color: Since block hashes start with zeroes, maybe one could shard based off the last two bytes:
Genesis block ends up in something like:
`$DATADIR/blocks/e2/6f/000000000019d6689c085ae165831e934ff763ae46a2a6c172b3f1b60a8ce26f.dat`
Block [896819](https://mempool.space/block/000000000000000
...
π¬ m3dwards commented on pull request "ci: remove 3rd party js from windows dll gha job":
(https://github.com/bitcoin/bitcoin/pull/32513#discussion_r2091989087)
Worked, force pushed [449cb9e](https://github.com/bitcoin/bitcoin/commit/449cb9e8985b350a96ddad74a9997ac95118edd0)
(https://github.com/bitcoin/bitcoin/pull/32513#discussion_r2091989087)
Worked, force pushed [449cb9e](https://github.com/bitcoin/bitcoin/commit/449cb9e8985b350a96ddad74a9997ac95118edd0)
π¬ davidgumberg commented on pull request "wallet: Keep track of the wallet's own transaction outputs in memory":
(https://github.com/bitcoin/bitcoin/pull/27286#discussion_r2092020703)
Agreed
(https://github.com/bitcoin/bitcoin/pull/27286#discussion_r2092020703)
Agreed
π¬ mzumsande commented on pull request "validation: ensure assumevalid is always used during reindex":
(https://github.com/bitcoin/bitcoin/pull/31615#discussion_r2091928728)
nit: I'd replace "here" with "before setting m_blockfiles_indexed" so the reason becomes more clear.
(https://github.com/bitcoin/bitcoin/pull/31615#discussion_r2091928728)
nit: I'd replace "here" with "before setting m_blockfiles_indexed" so the reason becomes more clear.
π¬ mzumsande commented on pull request "validation: ensure assumevalid is always used during reindex":
(https://github.com/bitcoin/bitcoin/pull/31615#discussion_r2092011943)
would be good to add a subtest that a run without `-assumevalid` will not connect the invalid block.
(https://github.com/bitcoin/bitcoin/pull/31615#discussion_r2092011943)
would be good to add a subtest that a run without `-assumevalid` will not connect the invalid block.
π¬ mzumsande commented on pull request "validation: ensure assumevalid is always used during reindex":
(https://github.com/bitcoin/bitcoin/pull/31615#discussion_r2091930195)
I'd prefer to call it `activate_all_chainstates` to not introduce a less precise term ("update").
(https://github.com/bitcoin/bitcoin/pull/31615#discussion_r2091930195)
I'd prefer to call it `activate_all_chainstates` to not introduce a less precise term ("update").
π¬ mzumsande commented on pull request "validation: ensure assumevalid is always used during reindex":
(https://github.com/bitcoin/bitcoin/pull/31615#discussion_r2091983914)
This means that in case of a reindex, `ActivateBestChain()`, will be called twice now from `ImportBlocks`, but that shouldn't be a problem because the second call just won't do any work.
(https://github.com/bitcoin/bitcoin/pull/31615#discussion_r2091983914)
This means that in case of a reindex, `ActivateBestChain()`, will be called twice now from `ImportBlocks`, but that shouldn't be a problem because the second call just won't do any work.
π¬ mzumsande commented on pull request "validation: ensure assumevalid is always used during reindex":
(https://github.com/bitcoin/bitcoin/pull/31615#discussion_r2092020053)
is there a reason we need to mine that many blocks, and not just a few more to make the test faster?
(https://github.com/bitcoin/bitcoin/pull/31615#discussion_r2092020053)
is there a reason we need to mine that many blocks, and not just a few more to make the test faster?
π¬ mzumsande commented on pull request "validation: ensure assumevalid is always used during reindex":
(https://github.com/bitcoin/bitcoin/pull/31615#discussion_r2092014654)
Comment needs updating, `-assumevalid` points to the last block, not the invalid one.
(https://github.com/bitcoin/bitcoin/pull/31615#discussion_r2092014654)
Comment needs updating, `-assumevalid` points to the last block, not the invalid one.
π¬ davidgumberg commented on pull request "cmake: Add application manifests when cross-compiling for Windows":
(https://github.com/bitcoin/bitcoin/pull/32396#discussion_r2092025994)
Question: Why is it necessary to search for mt.exe here, but not in the `windows-native-dll` job above?
(https://github.com/bitcoin/bitcoin/pull/32396#discussion_r2092025994)
Question: Why is it necessary to search for mt.exe here, but not in the `windows-native-dll` job above?
π¬ fkorotkov commented on issue "Revisiting us self-hosting parts of our CI":
(https://github.com/bitcoin/bitcoin/issues/31965#issuecomment-2885174918)
Cirrus CI founder here. Just wanted to add 2 cents that we now also have a service of self-hosted runners called [Cirrus Runners](https://cirrus-runners.app/) with fast machines and bigger caches. Main difference is also that it's fixed price for unlimited minutes per month hence we buy servers off traditional cloud like AWS/GCP/Azure.
(https://github.com/bitcoin/bitcoin/issues/31965#issuecomment-2885174918)
Cirrus CI founder here. Just wanted to add 2 cents that we now also have a service of self-hosted runners called [Cirrus Runners](https://cirrus-runners.app/) with fast machines and bigger caches. Main difference is also that it's fixed price for unlimited minutes per month hence we buy servers off traditional cloud like AWS/GCP/Azure.
π¬ Drew72-ita commented on issue "LevelDB error: Corruption: block checksum mismatch didn't trigger reindex.":
(https://github.com/bitcoin/bitcoin/issues/30138#issuecomment-2885200231)
I recently encountered this exact issue on a production Raspibolt node. Bitcoin Core was stuck in a restart loop due to a sudden corrupted txindex file (*.ldb) (cosmic ray XD?) . It wasnβt immediately obvious what was going onβthe logs showed a LevelDB error, but there was no clear indication that I needed to delete the index manually or restart with -reindex.
It took me quite some time to understand that a manual cleanup was required. At the very least, the log message should clearly explain t
...
(https://github.com/bitcoin/bitcoin/issues/30138#issuecomment-2885200231)
I recently encountered this exact issue on a production Raspibolt node. Bitcoin Core was stuck in a restart loop due to a sudden corrupted txindex file (*.ldb) (cosmic ray XD?) . It wasnβt immediately obvious what was going onβthe logs showed a LevelDB error, but there was no clear indication that I needed to delete the index manually or restart with -reindex.
It took me quite some time to understand that a manual cleanup was required. At the very least, the log message should clearly explain t
...
π€ ismaelsadeeq reviewed a pull request: "qa: feature_framework_startup_failures.py fixes & improvements (#30660 follow-up)"
(https://github.com/bitcoin/bitcoin/pull/32509#pullrequestreview-2845113491)
Concept ACK
thanks for following up.
(https://github.com/bitcoin/bitcoin/pull/32509#pullrequestreview-2845113491)
Concept ACK
thanks for following up.
π¬ ismaelsadeeq commented on pull request "qa: feature_framework_startup_failures.py fixes & improvements (#30660 follow-up)":
(https://github.com/bitcoin/bitcoin/pull/32509#discussion_r2092055590)
In "qa: Handle --timeout-factor=0" 1235a0df5ff6278d07dc95e7acc3ccb14f536286 whats the rationale behind multiplying by factor of 60?
Wondering if this is the right fix? if I pass timeout-factor=99998 the test will fail, but on master I dont think running the functional test with timeout-factor=99998 will trigger any failure.
(https://github.com/bitcoin/bitcoin/pull/32509#discussion_r2092055590)
In "qa: Handle --timeout-factor=0" 1235a0df5ff6278d07dc95e7acc3ccb14f536286 whats the rationale behind multiplying by factor of 60?
Wondering if this is the right fix? if I pass timeout-factor=99998 the test will fail, but on master I dont think running the functional test with timeout-factor=99998 will trigger any failure.
π¬ ismaelsadeeq commented on pull request "qa: feature_framework_startup_failures.py fixes & improvements (#30660 follow-up)":
(https://github.com/bitcoin/bitcoin/pull/32509#discussion_r2092055972)
Nice this is much better I think.
(https://github.com/bitcoin/bitcoin/pull/32509#discussion_r2092055972)
Nice this is much better I think.
π¬ ryanofsky commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2091927852)
re: https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2091450477
I wouldn't necessarily expect a user looking for help to know whether they are supposed to type `bitcoin help` or `bitcoin --help` or something else, so it seems reasonable to support both or these, as long as they are both fit in with other syntax we support. `git` could be a model here as well since it also supports both `git --help` and `git help`.
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2091927852)
re: https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2091450477
I wouldn't necessarily expect a user looking for help to know whether they are supposed to type `bitcoin help` or `bitcoin --help` or something else, so it seems reasonable to support both or these, as long as they are both fit in with other syntax we support. `git` could be a model here as well since it also supports both `git --help` and `git help`.
π¬ ryanofsky commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2091881632)
re: https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2091131888
> Shouldn't we throw when that happen?
Yes, that's a good idea, added this.
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2091881632)
re: https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2091131888
> Shouldn't we throw when that happen?
Yes, that's a good idea, added this.
π€ ryanofsky reviewed a pull request: "multiprocess: Add bitcoin wrapper executable"
(https://github.com/bitcoin/bitcoin/pull/31375#pullrequestreview-2844819831)
Thanks for the reviews!
Updated 7af6e1089ea264e870b26ac83e81e7aa374acbe1 -> a5ac43d98d1ad3ebed934f2c50208a85aae17e5e ([`pr/wrap.33`](https://github.com/ryanofsky/bitcoin/commits/pr/wrap.33) -> [`pr/wrap.34`](https://github.com/ryanofsky/bitcoin/commits/pr/wrap.34), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/wrap.33..pr/wrap.34)) just updating copyright and adding exception to handle an unexpected condition
re: https://github.com/bitcoin/bitcoin/pull/31375#pullrequestreview-2
...
(https://github.com/bitcoin/bitcoin/pull/31375#pullrequestreview-2844819831)
Thanks for the reviews!
Updated 7af6e1089ea264e870b26ac83e81e7aa374acbe1 -> a5ac43d98d1ad3ebed934f2c50208a85aae17e5e ([`pr/wrap.33`](https://github.com/ryanofsky/bitcoin/commits/pr/wrap.33) -> [`pr/wrap.34`](https://github.com/ryanofsky/bitcoin/commits/pr/wrap.34), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/wrap.33..pr/wrap.34)) just updating copyright and adding exception to handle an unexpected condition
re: https://github.com/bitcoin/bitcoin/pull/31375#pullrequestreview-2
...