👍 ryanofsky approved a pull request: "ci: skip Github CI on branch pushes for forks"
(https://github.com/bitcoin/bitcoin/pull/30487#pullrequestreview-2243000916)
Code review ACK 3067cd55f0fc79a7fa6342ca0600472cabcf2690.
Thanks for the explanation, that all sounds good to me.
The new description is definitely better than the original but it is not actually saying what behavior will be after this PR. Would be helpful to say something like "After this PR, pushes made to git branches inside fork repositories will no longer trigger CI runs, unless the git branches are associated with PRs in the fork repository, or the main repository." (assuming that is
...
(https://github.com/bitcoin/bitcoin/pull/30487#pullrequestreview-2243000916)
Code review ACK 3067cd55f0fc79a7fa6342ca0600472cabcf2690.
Thanks for the explanation, that all sounds good to me.
The new description is definitely better than the original but it is not actually saying what behavior will be after this PR. Would be helpful to say something like "After this PR, pushes made to git branches inside fork repositories will no longer trigger CI runs, unless the git branches are associated with PRs in the fork repository, or the main repository." (assuming that is
...
🤔 maflcko reviewed a pull request: "refactor: Replace ParseHex with consteval HexLiteral"
(https://github.com/bitcoin/bitcoin/pull/30377#pullrequestreview-2242829972)
review ACK 44bb5a12c4389dc18e181387356901787844e89f
(I'll do the rest later)
(https://github.com/bitcoin/bitcoin/pull/30377#pullrequestreview-2242829972)
review ACK 44bb5a12c4389dc18e181387356901787844e89f
(I'll do the rest later)
💬 maflcko commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1719924332)
nit in https://github.com/bitcoin/bitcoin/commit/1dddb4fb0ff88162e58e50e56d5e889bb5727a81: Wrong rename?
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1719924332)
nit in https://github.com/bitcoin/bitcoin/commit/1dddb4fb0ff88162e58e50e56d5e889bb5727a81: Wrong rename?
💬 maflcko commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1719915623)
nit in https://github.com/bitcoin/bitcoin/commit/1dddb4fb0ff88162e58e50e56d5e889bb5727a81: Use `std::span` to avoid having to touch this again in the future for that reason?
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1719915623)
nit in https://github.com/bitcoin/bitcoin/commit/1dddb4fb0ff88162e58e50e56d5e889bb5727a81: Use `std::span` to avoid having to touch this again in the future for that reason?
💬 maflcko commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1720015406)
nit in 82c9080f235f6ffbe983b1360dc5741ce95e9a7d: Follow-up nit: I think it was me who suggested to avoid array/span serialization in script, but I think it could be considered when the vector serialization is mirrored. I understand that this different from the `serialize.h` serialization, but the two are separate anyway (and script inherits the whole prevector functions to insert raw bytes at any point anyway).
In any case, this doesn't remove the need for `Vec` and can be done in a follow-up
...
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1720015406)
nit in 82c9080f235f6ffbe983b1360dc5741ce95e9a7d: Follow-up nit: I think it was me who suggested to avoid array/span serialization in script, but I think it could be considered when the vector serialization is mirrored. I understand that this different from the `serialize.h` serialization, but the two are separate anyway (and script inherits the whole prevector functions to insert raw bytes at any point anyway).
In any case, this doesn't remove the need for `Vec` and can be done in a follow-up
...
💬 maflcko commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1719910914)
nit in 1dddb4fb0ff88162e58e50e56d5e889bb5727a81: Any reason to remove the comment? Seems important to keep, otherwise a dev may be surprised when the function crashes when it isn't exactly 32 bytes.
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1719910914)
nit in 1dddb4fb0ff88162e58e50e56d5e889bb5727a81: Any reason to remove the comment? Seems important to keep, otherwise a dev may be surprised when the function crashes when it isn't exactly 32 bytes.
💬 maflcko commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1719917818)
nit in https://github.com/bitcoin/bitcoin/commit/1dddb4fb0ff88162e58e50e56d5e889bb5727a81: Use the safe `UCharCast` while touching this, instead of the possibly unsafe pointer cast?
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1719917818)
nit in https://github.com/bitcoin/bitcoin/commit/1dddb4fb0ff88162e58e50e56d5e889bb5727a81: Use the safe `UCharCast` while touching this, instead of the possibly unsafe pointer cast?
🤔 glozow reviewed a pull request: "wallet: fix blank legacy detection"
(https://github.com/bitcoin/bitcoin/pull/30621#pullrequestreview-2243021368)
ACK 6ed424f2db609f9f39ec1d1da2077c7616f3a0c2
(https://github.com/bitcoin/bitcoin/pull/30621#pullrequestreview-2243021368)
ACK 6ed424f2db609f9f39ec1d1da2077c7616f3a0c2
🚀 glozow merged a pull request: "wallet: fix blank legacy detection"
(https://github.com/bitcoin/bitcoin/pull/30621)
(https://github.com/bitcoin/bitcoin/pull/30621)
💬 fjahr commented on pull request "seeds: Pull additional nodes from my seeder and update fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/30008#discussion_r1720045465)
> Can that repo somehow have a permalink to the most recent asmap? Otherwise we would run into the same problem with outdated asmaps.
I hadn't really thought about this so far. This is a bit hacky but for now I copied the latest asmap file there to `latest_asmap.dat` and I would update that file as we create new maps so [this link](https://github.com/fjahr/asmap-data/raw/main/latest_asmap.dat) should always point to the latest file. I will look into making this a bit nicer by having latest.as
...
(https://github.com/bitcoin/bitcoin/pull/30008#discussion_r1720045465)
> Can that repo somehow have a permalink to the most recent asmap? Otherwise we would run into the same problem with outdated asmaps.
I hadn't really thought about this so far. This is a bit hacky but for now I copied the latest asmap file there to `latest_asmap.dat` and I would update that file as we create new maps so [this link](https://github.com/fjahr/asmap-data/raw/main/latest_asmap.dat) should always point to the latest file. I will look into making this a bit nicer by having latest.as
...
💬 0xB10C commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2293782491)
I tested with 67b1e236334f38ec4e4d2251dbdfb790f20ed88b that I'm able to do a nix build of the `bitcoind` (no GUI) and `bitcoin` (GUI) nix packages with the following modifications to the current nix package: https://github.com/0xB10C/nixpkgs/commit/fb77e0cf5194e48563e58eb418de8c2f9e0b48de. I also tested doing a development build with https://github.com/0xB10C/nix-bitcoin-core.
(https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2293782491)
I tested with 67b1e236334f38ec4e4d2251dbdfb790f20ed88b that I'm able to do a nix build of the `bitcoind` (no GUI) and `bitcoin` (GUI) nix packages with the following modifications to the current nix package: https://github.com/0xB10C/nixpkgs/commit/fb77e0cf5194e48563e58eb418de8c2f9e0b48de. I also tested doing a development build with https://github.com/0xB10C/nix-bitcoin-core.
💬 achow101 commented on pull request "seeds: Pull additional nodes from my seeder and update fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/30008#discussion_r1720070423)
I think you can make a symlink in the repo and just make sure it's updated every time.
(https://github.com/bitcoin/bitcoin/pull/30008#discussion_r1720070423)
I think you can make a symlink in the repo and just make sure it's updated every time.
💬 achow101 commented on pull request "seeds: Pull additional nodes from my seeder and update fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/30008#discussion_r1720088411)
Changed to use that asmap, although did not regenerate the seeds.
(https://github.com/bitcoin/bitcoin/pull/30008#discussion_r1720088411)
Changed to use that asmap, although did not regenerate the seeds.
📝 mzumsande opened a pull request: "validation: improve m_best_header tracking"
(https://github.com/bitcoin/bitcoin/pull/30666)
`m_best_header` (the most-work header not known to be on an invalid chain) can be wrong in the context of invalidation / reconsideration of blocks. This can happen naturally (a valid header is received and stored in our block tree db; when the full block arrives, it is found to be invalid) or triggered by the user with the `invalidateblock` / `reconsiderblock` rpc.
We don't currently use `m_best_header` for any critical things (see OP of #16974 for a list that still seem up-to-date), so it be
...
(https://github.com/bitcoin/bitcoin/pull/30666)
`m_best_header` (the most-work header not known to be on an invalid chain) can be wrong in the context of invalidation / reconsideration of blocks. This can happen naturally (a valid header is received and stored in our block tree db; when the full block arrives, it is found to be invalid) or triggered by the user with the `invalidateblock` / `reconsiderblock` rpc.
We don't currently use `m_best_header` for any critical things (see OP of #16974 for a list that still seem up-to-date), so it be
...
💬 mzumsande commented on pull request "validation: Improve, document and test logic for chains building on invalid blocks":
(https://github.com/bitcoin/bitcoin/pull/30207#issuecomment-2293902754)
I'll close this for now, large parts of this PR are contained in #30666 which actually fixes m_best_header tracking instead of just documenting its limitations.
The last two commits of this PR are not included in #30666 and fix a separate issue - I might open a separate PR for this at a later time.
(https://github.com/bitcoin/bitcoin/pull/30207#issuecomment-2293902754)
I'll close this for now, large parts of this PR are contained in #30666 which actually fixes m_best_header tracking instead of just documenting its limitations.
The last two commits of this PR are not included in #30666 and fix a separate issue - I might open a separate PR for this at a later time.
✅ mzumsande closed a pull request: "validation: Improve, document and test logic for chains building on invalid blocks"
(https://github.com/bitcoin/bitcoin/pull/30207)
(https://github.com/bitcoin/bitcoin/pull/30207)
💬 sipa commented on pull request "validation: improve m_best_header tracking":
(https://github.com/bitcoin/bitcoin/pull/30666#discussion_r1720132804)
Is `m_best_header` necessarily a descendant of `pindexNew` here? If not, this loop will walk past genesis.
(https://github.com/bitcoin/bitcoin/pull/30666#discussion_r1720132804)
Is `m_best_header` necessarily a descendant of `pindexNew` here? If not, this loop will walk past genesis.
💬 sipa commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#issuecomment-2294005237)
(Reposting at PR level instead of review level)
@glozow Cool, building on that:
```
$ git rebase --exec='F=$(mktemp) && make -j -C src bench/bench_bitcoin >/dev/null && src/bench/bench_bitcoin -filter=Linearize64TxWorstCase.* -min-time=60000 -output-json=$F >/dev/null 2>/dev/null && NS=$(cat $F | jq "(.results[0][\"median(elapsed)\"] - .results[1][\"median(elapsed)\"]) / 10000 * 1000000000") && TITLE=$(git show -s --format="%s") && echo "$NS $TITLE"' HEAD~11 2>/dev/null
11.60165442504903
...
(https://github.com/bitcoin/bitcoin/pull/30286#issuecomment-2294005237)
(Reposting at PR level instead of review level)
@glozow Cool, building on that:
```
$ git rebase --exec='F=$(mktemp) && make -j -C src bench/bench_bitcoin >/dev/null && src/bench/bench_bitcoin -filter=Linearize64TxWorstCase.* -min-time=60000 -output-json=$F >/dev/null 2>/dev/null && NS=$(cat $F | jq "(.results[0][\"median(elapsed)\"] - .results[1][\"median(elapsed)\"]) / 10000 * 1000000000") && TITLE=$(git show -s --format="%s") && echo "$NS $TITLE"' HEAD~11 2>/dev/null
11.60165442504903
...
💬 sipa commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1720227562)
Ok, mystery solved. These adhoc initialization costs do not interact with the `sqrt(2^k)+1` rule, because that rule is at the per-candidate-set level, while the adhoc costs are applied at the linearization level. It does in theory interact with the derived $\sqrt{12 * 2^n}+n$ rule, but for $n \leq 3$ the linearization code is optimal without any search, and for $n \geq 4$, the formula is sufficiently overshooting to accomodate the ad-hoc costs. Still, I've changed the test to use precomputed lim
...
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1720227562)
Ok, mystery solved. These adhoc initialization costs do not interact with the `sqrt(2^k)+1` rule, because that rule is at the per-candidate-set level, while the adhoc costs are applied at the linearization level. It does in theory interact with the derived $\sqrt{12 * 2^n}+n$ rule, but for $n \leq 3$ the linearization code is optimal without any search, and for $n \geq 4$, the formula is sufficiently overshooting to accomodate the ad-hoc costs. Still, I've changed the test to use precomputed lim
...
💬 hebasto commented on pull request "fuzz: a target for the block index database":
(https://github.com/bitcoin/bitcoin/pull/28209#issuecomment-2294101230)
Ported to the CMake-based build system in https://github.com/hebasto/bitcoin/pull/334.
(https://github.com/bitcoin/bitcoin/pull/28209#issuecomment-2294101230)
Ported to the CMake-based build system in https://github.com/hebasto/bitcoin/pull/334.