💬 rkrux commented on issue "28.0 RC Testing Guide Feedback":
(https://github.com/bitcoin/bitcoin/issues/30854#issuecomment-2349306179)
Thanks @tdb3 for trying the guide out and sharing your feedback. I think I addressed all of it, and good that you raised the last point - the previous transaction details being passed in the sign command would not be apparent to anyone following the guide, mentioning that in the guide is helpful.
(https://github.com/bitcoin/bitcoin/issues/30854#issuecomment-2349306179)
Thanks @tdb3 for trying the guide out and sharing your feedback. I think I addressed all of it, and good that you raised the last point - the previous transaction details being passed in the sign command would not be apparent to anyone following the guide, mentioning that in the guide is helpful.
💬 achow101 commented on pull request "streams: cache file position within AutoFile":
(https://github.com/bitcoin/bitcoin/pull/30884#issuecomment-2349311621)
ACK e624a9bef16b6335fd119c10698352b59bf2930a
> Extending the `AutoFile` interface so that `AutoFile::Get` is no longer needed can be done as a follow-up.
It does seem a bit fragile to still have `AutoFile::Get`. I would okay with backporting additional commits that remove it if they are not too complicated.
(https://github.com/bitcoin/bitcoin/pull/30884#issuecomment-2349311621)
ACK e624a9bef16b6335fd119c10698352b59bf2930a
> Extending the `AutoFile` interface so that `AutoFile::Get` is no longer needed can be done as a follow-up.
It does seem a bit fragile to still have `AutoFile::Get`. I would okay with backporting additional commits that remove it if they are not too complicated.
💬 hebasto commented on pull request "[28.x] Further backports and rc2":
(https://github.com/bitcoin/bitcoin/pull/30827#issuecomment-2349312031)
@achow101
Feel free to pick [this](https://github.com/hebasto/bitcoin/commit/be6354db410d162f82b5e3c76d964c0365e51d96) translation update, which resolves https://github.com/bitcoin/bitcoin/issues/30897 and includes new translations made over the nearly 3 weeks since https://github.com/bitcoin/bitcoin/pull/30715.
(https://github.com/bitcoin/bitcoin/pull/30827#issuecomment-2349312031)
@achow101
Feel free to pick [this](https://github.com/hebasto/bitcoin/commit/be6354db410d162f82b5e3c76d964c0365e51d96) translation update, which resolves https://github.com/bitcoin/bitcoin/issues/30897 and includes new translations made over the nearly 3 weeks since https://github.com/bitcoin/bitcoin/pull/30715.
💬 pablomartin4btc commented on issue "translations: `28.x` update pulled in random strings?":
(https://github.com/bitcoin/bitcoin/issues/30897#issuecomment-2349313438)
In the meantime, is it too ugly to add a regex into `update-translations.py`? (or a function like the existent ones that already parse the strings and validate their format)
```
# regex patterns for malicious content and symbols (just an example)
MALICIOUS_PATTERN = re.compile(r'[\x00-\x1F\x7F-\x9F<>&\'";`\\\xFFFD]|'
r'(\.\./|\.\.\\|\%2e%2e/|\%2e%2e\\|'
r'\$HOME/|\%USERPROFILE\%|\%APPDATA\%|\$USER|\$PATH|'
...
(https://github.com/bitcoin/bitcoin/issues/30897#issuecomment-2349313438)
In the meantime, is it too ugly to add a regex into `update-translations.py`? (or a function like the existent ones that already parse the strings and validate their format)
```
# regex patterns for malicious content and symbols (just an example)
MALICIOUS_PATTERN = re.compile(r'[\x00-\x1F\x7F-\x9F<>&\'";`\\\xFFFD]|'
r'(\.\./|\.\.\\|\%2e%2e/|\%2e%2e\\|'
r'\$HOME/|\%USERPROFILE\%|\%APPDATA\%|\$USER|\$PATH|'
...
💬 dergoegge commented on pull request "fuzz: Test headers pre-sync through p2p":
(https://github.com/bitcoin/bitcoin/pull/30661#issuecomment-2349317870)
>Would it make sense to additionally have some belt-and-suspenders check somewhere at the start of bitcoind that FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION is not set, so it's not only in the build system?
I guess we could (e.g. in `bitcoind.cpp`) do something like:
```c++
#ifdef FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION
static_assert(false, "bitcoind can't be build with FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION");
#endif
```
But I would be surprised if our existing tests do not already
...
(https://github.com/bitcoin/bitcoin/pull/30661#issuecomment-2349317870)
>Would it make sense to additionally have some belt-and-suspenders check somewhere at the start of bitcoind that FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION is not set, so it's not only in the build system?
I guess we could (e.g. in `bitcoind.cpp`) do something like:
```c++
#ifdef FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION
static_assert(false, "bitcoind can't be build with FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION");
#endif
```
But I would be surprised if our existing tests do not already
...
💬 fanquake commented on pull request "[28.x] Further backports and rc2":
(https://github.com/bitcoin/bitcoin/pull/30827#issuecomment-2349321778)
> Feel free to pick [this](https://github.com/hebasto/bitcoin/commit/be6354db410d162f82b5e3c76d964c0365e51d96) translation update
There's not so much of a rush here that this should bypass the normal PR & review process. This should first be PR'd and merged into master and then backported. Otherwise any broken string are still in master. There is at least one other change waiting for 28.x, so rc2 also needs to either wait for that, or we'll be having an rc3 (which can include the ts update) i
...
(https://github.com/bitcoin/bitcoin/pull/30827#issuecomment-2349321778)
> Feel free to pick [this](https://github.com/hebasto/bitcoin/commit/be6354db410d162f82b5e3c76d964c0365e51d96) translation update
There's not so much of a rush here that this should bypass the normal PR & review process. This should first be PR'd and merged into master and then backported. Otherwise any broken string are still in master. There is at least one other change waiting for 28.x, so rc2 also needs to either wait for that, or we'll be having an rc3 (which can include the ts update) i
...
💬 instagibbs commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#issuecomment-2349326264)
@sdaftuar is that number of iterations per search invocation, or total over the entire linearization?
Could a version of this graph show the total fraction of *transactions* that are optimally linearized? Or maybe just do a chart where a cluster min size of <larger number> is used to ignore the many small clusters we know we can optimally linearize?
(https://github.com/bitcoin/bitcoin/pull/30286#issuecomment-2349326264)
@sdaftuar is that number of iterations per search invocation, or total over the entire linearization?
Could a version of this graph show the total fraction of *transactions* that are optimally linearized? Or maybe just do a chart where a cluster min size of <larger number> is used to ignore the many small clusters we know we can optimally linearize?
💬 hebasto commented on pull request "[28.x] Further backports and rc2":
(https://github.com/bitcoin/bitcoin/pull/30827#issuecomment-2349332544)
> There's not so much of a rush here that this should bypass the normal PR & review process.
Sure.
> This should first be PR'd and merged into master and then backported.
After branching off, the Transifex.com has translation for the version branches only. Fetching 28x translations into the master branch does not look reasonable.
> Otherwise any broken string are still in master.
Maybe this case is a special one.
(https://github.com/bitcoin/bitcoin/pull/30827#issuecomment-2349332544)
> There's not so much of a rush here that this should bypass the normal PR & review process.
Sure.
> This should first be PR'd and merged into master and then backported.
After branching off, the Transifex.com has translation for the version branches only. Fetching 28x translations into the master branch does not look reasonable.
> Otherwise any broken string are still in master.
Maybe this case is a special one.
💬 rkrux commented on issue "28.0 RC Testing Guide Feedback":
(https://github.com/bitcoin/bitcoin/issues/30854#issuecomment-2349335396)
@fjahr I've updated the guide adding instructions for assumeutxo mainnet.
(https://github.com/bitcoin/bitcoin/issues/30854#issuecomment-2349335396)
@fjahr I've updated the guide adding instructions for assumeutxo mainnet.
📝 hebasto opened a pull request: "qt: Translations update"
(https://github.com/bitcoin/bitcoin/pull/30899)
The recent translations from Transifex.com fetched with the bitcoin-maintainer-tools/update-translations.py tool.
Fixes https://github.com/bitcoin/bitcoin/issues/30897.
(https://github.com/bitcoin/bitcoin/pull/30899)
The recent translations from Transifex.com fetched with the bitcoin-maintainer-tools/update-translations.py tool.
Fixes https://github.com/bitcoin/bitcoin/issues/30897.
💬 hebasto commented on pull request "[28.x] Further backports and rc2":
(https://github.com/bitcoin/bitcoin/pull/30827#issuecomment-2349337355)
> > There's not so much of a rush here that this should bypass the normal PR & review process.
>
> Sure.
Please see https://github.com/bitcoin/bitcoin/pull/30899.
(https://github.com/bitcoin/bitcoin/pull/30827#issuecomment-2349337355)
> > There's not so much of a rush here that this should bypass the normal PR & review process.
>
> Sure.
Please see https://github.com/bitcoin/bitcoin/pull/30899.
🤔 jonatack reviewed a pull request: "doc: cmake: prepend "build" to functional/test_runner.py"
(https://github.com/bitcoin/bitcoin/pull/30859#pullrequestreview-2303553463)
Concept ACK. I came across this as well, and the new error message would indeed be helpful. (Bonus points if the message can be defined in one place, didn't check.)
(https://github.com/bitcoin/bitcoin/pull/30859#pullrequestreview-2303553463)
Concept ACK. I came across this as well, and the new error message would indeed be helpful. (Bonus points if the message can be defined in one place, didn't check.)
💬 jonatack commented on pull request "doc: cmake: prepend "build" to functional/test_runner.py":
(https://github.com/bitcoin/bitcoin/pull/30859#discussion_r1759154738)
```suggestion
raise Exception(f"config.ini file {self.options.configfile} not found, be sure to run this script from within the build directory")
```
(https://github.com/bitcoin/bitcoin/pull/30859#discussion_r1759154738)
```suggestion
raise Exception(f"config.ini file {self.options.configfile} not found, be sure to run this script from within the build directory")
```
💬 jonatack commented on pull request "doc: cmake: prepend "build" to functional/test_runner.py":
(https://github.com/bitcoin/bitcoin/pull/30859#discussion_r1759154535)
```suggestion
raise Exception(f"config.ini file {configfile} not found, be sure to run this script from within the build directory")
```
(https://github.com/bitcoin/bitcoin/pull/30859#discussion_r1759154535)
```suggestion
raise Exception(f"config.ini file {configfile} not found, be sure to run this script from within the build directory")
```
🤔 danielabrozzoni reviewed a pull request: "signet: fixing mining for OP_TRUE challenge"
(https://github.com/bitcoin/bitcoin/pull/29032#pullrequestreview-2303581034)
Concept ACK
> However contrib/signet/miner can't handle this, as it fails with PSBT signing failed.
I can't seem to have contrib/signet/miner fail on master, am I doing something wrong?
I'm on 1d5b2406bb9ce619219a3b76608bd764a3b162c3,
I'm testing manually with `bitcoind -signet -signetchallenge=51` and:
```
❯ $MINER --cli="$CLI" generate --grind-cmd="$GRIND" --address="$ADDR" --nbits=$NBITS --ongoing
2024-09-13 18:29:59 INFO Mined block at height 24; next in -30h15m33s (mine)
2024-09
...
(https://github.com/bitcoin/bitcoin/pull/29032#pullrequestreview-2303581034)
Concept ACK
> However contrib/signet/miner can't handle this, as it fails with PSBT signing failed.
I can't seem to have contrib/signet/miner fail on master, am I doing something wrong?
I'm on 1d5b2406bb9ce619219a3b76608bd764a3b162c3,
I'm testing manually with `bitcoind -signet -signetchallenge=51` and:
```
❯ $MINER --cli="$CLI" generate --grind-cmd="$GRIND" --address="$ADDR" --nbits=$NBITS --ongoing
2024-09-13 18:29:59 INFO Mined block at height 24; next in -30h15m33s (mine)
2024-09
...
💬 danielabrozzoni commented on pull request "signet: fixing mining for OP_TRUE challenge":
(https://github.com/bitcoin/bitcoin/pull/29032#discussion_r1759171230)
I would rephrase as "the walletprocesspsbt step *has* to be skipped", as it currently returns an error:
```
❯ $CLI -signet getblocktemplate '{"rules": ["signet","segwit"]}' | $MINER --cli="$CLI" genpsbt --address="$ADDR" | $CLI -signet -stdin walletprocesspsbt
error code: -22
error message:
Specified sighash value does not match value stored in PSBT
```
(https://github.com/bitcoin/bitcoin/pull/29032#discussion_r1759171230)
I would rephrase as "the walletprocesspsbt step *has* to be skipped", as it currently returns an error:
```
❯ $CLI -signet getblocktemplate '{"rules": ["signet","segwit"]}' | $MINER --cli="$CLI" genpsbt --address="$ADDR" | $CLI -signet -stdin walletprocesspsbt
error code: -22
error message:
Specified sighash value does not match value stored in PSBT
```
🤔 jonatack reviewed a pull request: "kernel: Move background load thread to node context"
(https://github.com/bitcoin/bitcoin/pull/30896#pullrequestreview-2303608061)
Light ACK bc7900f33db3d01fb93dfee7981c01ea495cd42e
Good further separation of concerns.
(https://github.com/bitcoin/bitcoin/pull/30896#pullrequestreview-2303608061)
Light ACK bc7900f33db3d01fb93dfee7981c01ea495cd42e
Good further separation of concerns.
💬 ryanofsky commented on pull request "Fix crash when closing wallet":
(https://github.com/bitcoin-core/gui/pull/835#issuecomment-2349397769)
Code review ACK a965f2bc07a3588f8c2b8d6a542961562e3f5d0e
This seems like a workable fix, but I think it it is not ideal. The underlying problem here is that https://github.com/bitcoin/bitcoin/pull/30659 moved `NotifyUnload` from `FlushAndDeleteWallet` to `RemoveWallet`, so now instead of `NotifyUnload` guaranteed being called once, it can now be called multiple times, but the GUI code assumes it is only called once and double deletes the wallet model and crashes. I think a better for this is
...
(https://github.com/bitcoin-core/gui/pull/835#issuecomment-2349397769)
Code review ACK a965f2bc07a3588f8c2b8d6a542961562e3f5d0e
This seems like a workable fix, but I think it it is not ideal. The underlying problem here is that https://github.com/bitcoin/bitcoin/pull/30659 moved `NotifyUnload` from `FlushAndDeleteWallet` to `RemoveWallet`, so now instead of `NotifyUnload` guaranteed being called once, it can now be called multiple times, but the GUI code assumes it is only called once and double deletes the wallet model and crashes. I think a better for this is
...
💬 pablomartin4btc commented on pull request "Fix crash when closing wallet":
(https://github.com/bitcoin-core/gui/pull/835#issuecomment-2349418124)
> now instead of NotifyUnload guaranteed being called once, it can now be called multiple times, but the GUI code assumes it is only called once and double deletes the wallet model and crashes.
How can I repro the crash?
(https://github.com/bitcoin-core/gui/pull/835#issuecomment-2349418124)
> now instead of NotifyUnload guaranteed being called once, it can now be called multiple times, but the GUI code assumes it is only called once and double deletes the wallet model and crashes.
How can I repro the crash?
💬 ryanofsky commented on pull request "Fix crash when closing wallet":
(https://github.com/bitcoin-core/gui/pull/835#issuecomment-2349438972)
> How can I repro the crash?
I don't know of an easy way to reproduce the crash after the current fix, but it is possible to revert the current and test the alternate suggest fix by using the same steps in the PR description, choosing close wallet from the GUI menu.
It might also be possible to make the GUI after the current fix by enabling RPC and calling unloadwallet RPC simultaneously from multiple threads, but not sure about that.
(https://github.com/bitcoin-core/gui/pull/835#issuecomment-2349438972)
> How can I repro the crash?
I don't know of an easy way to reproduce the crash after the current fix, but it is possible to revert the current and test the alternate suggest fix by using the same steps in the PR description, choosing close wallet from the GUI menu.
It might also be possible to make the GUI after the current fix by enabling RPC and calling unloadwallet RPC simultaneously from multiple threads, but not sure about that.