💬 fjahr commented on pull request "init: Require explicit -asmap filename":
(https://github.com/bitcoin/bitcoin/pull/33770#issuecomment-3564836443)
> @fjahr DrahtBot is ignoring your ACK, is that intended? If you mean to withdraw your ACK, I think editing your comment with a strikethrough and explanation is the typical way people do that.
It falsely interpreted this comment as an ACK previously and that's when I put the ignore: https://github.com/bitcoin/bitcoin/pull/33770#issuecomment-3497304815 I have removed the emoji reaction, not sure if that makes it reconsider my latest ACK but that is still valid.
(https://github.com/bitcoin/bitcoin/pull/33770#issuecomment-3564836443)
> @fjahr DrahtBot is ignoring your ACK, is that intended? If you mean to withdraw your ACK, I think editing your comment with a strikethrough and explanation is the typical way people do that.
It falsely interpreted this comment as an ACK previously and that's when I put the ignore: https://github.com/bitcoin/bitcoin/pull/33770#issuecomment-3497304815 I have removed the emoji reaction, not sure if that makes it reconsider my latest ACK but that is still valid.
💬 fjahr commented on pull request "init: Require explicit -asmap filename":
(https://github.com/bitcoin/bitcoin/pull/33770#issuecomment-3564842717)
re-ACK 288b8c30be42f2879d1a1f3d5ec3cac13f87ace4
Only minor fixes since last review.
(https://github.com/bitcoin/bitcoin/pull/33770#issuecomment-3564842717)
re-ACK 288b8c30be42f2879d1a1f3d5ec3cac13f87ace4
Only minor fixes since last review.
💬 fjahr commented on pull request "init: Split file path handling out of -asmap option":
(https://github.com/bitcoin/bitcoin/pull/33631#issuecomment-3564889500)
> I'm slightly confused as to what the plan is between this and #33770. This PR both has code conflicts and logical conflicts with #33770. 33770 redefines `-asmap` in one way, while this redefines it in the opposite way.
It is not the opposite way, it's an orthogonal issue. #33770 removes the usage of the default file and this PR splits the `-asmap` usage from being a path arg (maybe interpreted as a bool) to a bool arg and a path arg seperately which is much cleaner compared to master (with
...
(https://github.com/bitcoin/bitcoin/pull/33631#issuecomment-3564889500)
> I'm slightly confused as to what the plan is between this and #33770. This PR both has code conflicts and logical conflicts with #33770. 33770 redefines `-asmap` in one way, while this redefines it in the opposite way.
It is not the opposite way, it's an orthogonal issue. #33770 removes the usage of the default file and this PR splits the `-asmap` usage from being a path arg (maybe interpreted as a bool) to a bool arg and a path arg seperately which is much cleaner compared to master (with
...
💬 achow101 commented on pull request "doc: update multisig tutorial to use multipath descriptors":
(https://github.com/bitcoin/bitcoin/pull/33286#issuecomment-3564906970)
ACK de7c3587cd4586bbed94a4ea6eae4a252301daee
(https://github.com/bitcoin/bitcoin/pull/33286#issuecomment-3564906970)
ACK de7c3587cd4586bbed94a4ea6eae4a252301daee
💬 sipa commented on pull request "init: Split file path handling out of -asmap option":
(https://github.com/bitcoin/bitcoin/pull/33631#issuecomment-3564910471)
I think having two options to control such a minor feature is unintuive and overkill. If that's what people like, no objection as it really isn't important. But my preference would be to put everything in the `-asmap` option:
* `-asmap=none` (and for compatibility, `-asmap=0`) to get the /16 based splitting.
* `-asmap=builtin` to get the file built-in to the binary, or the same as `none` if nothing was built in.
* `-asmap=<filename>` to get the specified filename if it exists, and error oth
...
(https://github.com/bitcoin/bitcoin/pull/33631#issuecomment-3564910471)
I think having two options to control such a minor feature is unintuive and overkill. If that's what people like, no objection as it really isn't important. But my preference would be to put everything in the `-asmap` option:
* `-asmap=none` (and for compatibility, `-asmap=0`) to get the /16 based splitting.
* `-asmap=builtin` to get the file built-in to the binary, or the same as `none` if nothing was built in.
* `-asmap=<filename>` to get the specified filename if it exists, and error oth
...
💬 fjahr commented on pull request "init: Split file path handling out of -asmap option":
(https://github.com/bitcoin/bitcoin/pull/33631#issuecomment-3564915518)
I guess there are too many walls of text here and elsewhere so I will make it more concise, this PR here (and https://github.com/bitcoin/bitcoin/pull/33632 with a different approach) fixes two things:
1. `bitcoind -asmap=1` => errors because it looks for a file named "1" in datadir
2. You can not use asmap=1 in the config file for the same reason
https://github.com/bitcoin/bitcoin/pull/33770 doesn't fix this, it just gets rid of the current reason anyone would use `-asmap=1`, to activate
...
(https://github.com/bitcoin/bitcoin/pull/33631#issuecomment-3564915518)
I guess there are too many walls of text here and elsewhere so I will make it more concise, this PR here (and https://github.com/bitcoin/bitcoin/pull/33632 with a different approach) fixes two things:
1. `bitcoind -asmap=1` => errors because it looks for a file named "1" in datadir
2. You can not use asmap=1 in the config file for the same reason
https://github.com/bitcoin/bitcoin/pull/33770 doesn't fix this, it just gets rid of the current reason anyone would use `-asmap=1`, to activate
...
🚀 achow101 merged a pull request: "doc: update multisig tutorial to use multipath descriptors"
(https://github.com/bitcoin/bitcoin/pull/33286)
(https://github.com/bitcoin/bitcoin/pull/33286)
💬 achow101 commented on pull request "init: Require explicit -asmap filename":
(https://github.com/bitcoin/bitcoin/pull/33770#issuecomment-3564962500)
ACK 288b8c30be42f2879d1a1f3d5ec3cac13f87ace4
(https://github.com/bitcoin/bitcoin/pull/33770#issuecomment-3564962500)
ACK 288b8c30be42f2879d1a1f3d5ec3cac13f87ace4
🚀 achow101 merged a pull request: "init: Require explicit -asmap filename"
(https://github.com/bitcoin/bitcoin/pull/33770)
(https://github.com/bitcoin/bitcoin/pull/33770)
💬 ryanofsky commented on issue "should concurrent IPC requests directed to the same thread cause a crash?":
(https://github.com/bitcoin/bitcoin/issues/33923#issuecomment-3564979973)
> shouldn't Bitcoin Core simply not crash if two requests arrive for the same thread?
Yes this shouldn't crash. Hopefully, if you are using a version of bitcoin after https://github.com/bitcoin-core/libmultiprocess/pull/214 (part of #33518 in master and part of #33519 in 30.x branch) you should just see "thread busy" errors instead of crashes, and it is a bug if there are still crashes.
> ideally, a naive client should simply be forced to wait a bit longer if they send concurrent requests agai
...
(https://github.com/bitcoin/bitcoin/issues/33923#issuecomment-3564979973)
> shouldn't Bitcoin Core simply not crash if two requests arrive for the same thread?
Yes this shouldn't crash. Hopefully, if you are using a version of bitcoin after https://github.com/bitcoin-core/libmultiprocess/pull/214 (part of #33518 in master and part of #33519 in 30.x branch) you should just see "thread busy" errors instead of crashes, and it is a bug if there are still crashes.
> ideally, a naive client should simply be forced to wait a bit longer if they send concurrent requests agai
...
✅ ryanofsky closed an issue: "Default ASmap file path is not used unless -asmap is set"
(https://github.com/bitcoin/bitcoin/issues/33386)
(https://github.com/bitcoin/bitcoin/issues/33386)
💬 ryanofsky commented on issue "Default ASmap file path is not used unless -asmap is set":
(https://github.com/bitcoin/bitcoin/issues/33386#issuecomment-3565003991)
Closing since this should be resolved with #33770
(https://github.com/bitcoin/bitcoin/issues/33386#issuecomment-3565003991)
Closing since this should be resolved with #33770
💬 ryanofsky commented on pull request "init: Split file path handling out of -asmap option":
(https://github.com/bitcoin/bitcoin/pull/33631#issuecomment-3565097582)
Yes conflict with #33770 is superficial, and both could be merged. They both solved #33386 in different ways, but are otherwise independent.
I don't think this PR is a good idea, but don't oppose it if others like it. I don't think separating `-asmap` and `-asmapfile` options is user-friendly or consistent, even though I understand the problem this is trying to solve. I just think the problem is minor and temporary.
Right now since #33770 is merged there's no issue with the `-asmap` option
...
(https://github.com/bitcoin/bitcoin/pull/33631#issuecomment-3565097582)
Yes conflict with #33770 is superficial, and both could be merged. They both solved #33386 in different ways, but are otherwise independent.
I don't think this PR is a good idea, but don't oppose it if others like it. I don't think separating `-asmap` and `-asmapfile` options is user-friendly or consistent, even though I understand the problem this is trying to solve. I just think the problem is minor and temporary.
Right now since #33770 is merged there's no issue with the `-asmap` option
...
💬 sdaftuar commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#issuecomment-3565452558)
Just repushed this branch with `POST_CHANGE_WORK` reduced to `5 * ACCEPTABLE_ITERS`. I gathered some simulation data (by replaying transactions/blocks that my datalogger recorded in 2023) in this [gist](https://gist.github.com/sdaftuar/77d4e67d0b5e1ef59e6d2ecf4d43484e), if anyone is curious about the benchmarks I was looking at.
The main piece of information to consider is that in November and December 2023, there were clusters that CSS had trouble linearizing, and so the 99th percentile of
...
(https://github.com/bitcoin/bitcoin/pull/33629#issuecomment-3565452558)
Just repushed this branch with `POST_CHANGE_WORK` reduced to `5 * ACCEPTABLE_ITERS`. I gathered some simulation data (by replaying transactions/blocks that my datalogger recorded in 2023) in this [gist](https://gist.github.com/sdaftuar/77d4e67d0b5e1ef59e6d2ecf4d43484e), if anyone is curious about the benchmarks I was looking at.
The main piece of information to consider is that in November and December 2023, there were clusters that CSS had trouble linearizing, and so the 99th percentile of
...
💬 stratospher commented on pull request "validation: remove BLOCK_FAILED_CHILD":
(https://github.com/bitcoin/bitcoin/pull/32950#discussion_r2552304930)
@stringintech great catch! thank you! I've updated the PR to use the diff suggested above.
I forgot the 1 edge case scenario where this is possible - https://github.com/bitcoin/bitcoin/issues/32173#issuecomment-2767030982.
I ran bitcoind with an inconsistent block index state from the functional test in the issue (`BLOCK_FAILED_CHILD` happens with valid parents) and current logic wouldn't work as you mentioned. but it works on the latest push now.
(https://github.com/bitcoin/bitcoin/pull/32950#discussion_r2552304930)
@stringintech great catch! thank you! I've updated the PR to use the diff suggested above.
I forgot the 1 edge case scenario where this is possible - https://github.com/bitcoin/bitcoin/issues/32173#issuecomment-2767030982.
I ran bitcoind with an inconsistent block index state from the functional test in the issue (`BLOCK_FAILED_CHILD` happens with valid parents) and current logic wouldn't work as you mentioned. but it works on the latest push now.
💬 stratospher commented on pull request "validation: remove BLOCK_FAILED_CHILD":
(https://github.com/bitcoin/bitcoin/pull/32950#discussion_r2552305472)
now that I think about it again, makes sense to call the rest of the code with last disconnected block (and not pindex) for a consistent + graceful shutdown. thanks for catching this!
(https://github.com/bitcoin/bitcoin/pull/32950#discussion_r2552305472)
now that I think about it again, makes sense to call the rest of the code with last disconnected block (and not pindex) for a consistent + graceful shutdown. thanks for catching this!
💬 stratospher commented on pull request "validation: remove BLOCK_FAILED_CHILD":
(https://github.com/bitcoin/bitcoin/pull/32950#discussion_r2552305597)
taken. thanks!
(https://github.com/bitcoin/bitcoin/pull/32950#discussion_r2552305597)
taken. thanks!
💬 stratospher commented on pull request "validation: remove BLOCK_FAILED_CHILD":
(https://github.com/bitcoin/bitcoin/pull/32950#issuecomment-3565803301)
thanks for the great catches @stringintech and the nicer tests @stickies-v! I've pushed an [update](https://github.com/bitcoin/bitcoin/compare/cfadc8038c08f9804c81af7950164483761f1db5..b22d8b89c29a0a0d385d38d8ab74e5b940ddf7d5).
(https://github.com/bitcoin/bitcoin/pull/32950#issuecomment-3565803301)
thanks for the great catches @stringintech and the nicer tests @stickies-v! I've pushed an [update](https://github.com/bitcoin/bitcoin/compare/cfadc8038c08f9804c81af7950164483761f1db5..b22d8b89c29a0a0d385d38d8ab74e5b940ddf7d5).
💬 ajtowns commented on pull request "Relax standardness rules regarding CHECKMULTISIG":
(https://github.com/bitcoin/bitcoin/pull/33755#issuecomment-3565835341)
> Hi AJ, are these the test vectors you had in mind?
I was thinking functional tests, to also make sure there isn't some obscure policy rule that the unit tests miss that nevertheless ends up blocking successful spends of things that should be spendable. Unit tests are good too though.
"CMS NOT" isn't super interesting though, since it's easily satisfiable with empty sigs (apart from mempool/consensus inconsistency bugs). The things that (per this PR's arguments) should be spendable are `1
...
(https://github.com/bitcoin/bitcoin/pull/33755#issuecomment-3565835341)
> Hi AJ, are these the test vectors you had in mind?
I was thinking functional tests, to also make sure there isn't some obscure policy rule that the unit tests miss that nevertheless ends up blocking successful spends of things that should be spendable. Unit tests are good too though.
"CMS NOT" isn't super interesting though, since it's easily satisfiable with empty sigs (apart from mempool/consensus inconsistency bugs). The things that (per this PR's arguments) should be spendable are `1
...
💬 romanz commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2552443566)
Oops - thanks!
Fixed in [8457a71a64..78d6402458](https://github.com/bitcoin/bitcoin/compare/8457a71a6420792a1b9c7c8645c9e5c607a4b90c..78d6402458d7d14533444d893c989f0534a3896f) (also removed 2 duplicate test cases above).
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2552443566)
Oops - thanks!
Fixed in [8457a71a64..78d6402458](https://github.com/bitcoin/bitcoin/compare/8457a71a6420792a1b9c7c8645c9e5c607a4b90c..78d6402458d7d14533444d893c989f0534a3896f) (also removed 2 duplicate test cases above).
💬 musaHaruna commented on pull request "wallet, rpc: add UTXO set check and incremental rescan to importdescriptors":
(https://github.com/bitcoin/bitcoin/pull/33392#discussion_r2553065653)
Fixed.
(https://github.com/bitcoin/bitcoin/pull/33392#discussion_r2553065653)
Fixed.