⚠️ gmart7t2 opened an issue: "decoderawtransaction should use hex or decimal, not both"
(https://github.com/bitcoin/bitcoin/issues/30067)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
I have two different op_return outputs, but they are shown identically in their "asm" decoding:
```
"scriptPubKey": {
"asm": "OP_RETURN 1431655765",
"hex": "6a051431655765",
}
```
```
"scriptPubKey": {
"asm": "OP_RETURN 1431655765",
"hex": "6a0455555555",
}
```
One shows the hex value. The other shows the decimal va
...
(https://github.com/bitcoin/bitcoin/issues/30067)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
I have two different op_return outputs, but they are shown identically in their "asm" decoding:
```
"scriptPubKey": {
"asm": "OP_RETURN 1431655765",
"hex": "6a051431655765",
}
```
```
"scriptPubKey": {
"asm": "OP_RETURN 1431655765",
"hex": "6a0455555555",
}
```
One shows the hex value. The other shows the decimal va
...
💬 sipa commented on issue "decoderawtransaction should use hex or decimal, not both":
(https://github.com/bitcoin/bitcoin/issues/30067#issuecomment-2101692041)
Duplicate of #27795 ?
(https://github.com/bitcoin/bitcoin/issues/30067#issuecomment-2101692041)
Duplicate of #27795 ?
📝 kevkevinpal opened a pull request: "test: assert can't activate snapshot based chainstate more than once"
(https://github.com/bitcoin/bitcoin/pull/30068)
In ActivateSnapshot we return false if there already exists a snapshot-based chainstate this is a test that asserts that happens
This adds coverage to [this part of the codebase](https://github.com/bitcoin/bitcoin/blob/master/src/validation.cpp#L5477)
(https://github.com/bitcoin/bitcoin/pull/30068)
In ActivateSnapshot we return false if there already exists a snapshot-based chainstate this is a test that asserts that happens
This adds coverage to [this part of the codebase](https://github.com/bitcoin/bitcoin/blob/master/src/validation.cpp#L5477)
🚀 fanquake merged a pull request: "build, test: Remove unused `TIMEOUT` environment variable"
(https://github.com/bitcoin/bitcoin/pull/30063)
(https://github.com/bitcoin/bitcoin/pull/30063)
💬 edilmedeiros commented on issue "RFC: "Insufficient review" tag for closed PRs":
(https://github.com/bitcoin/bitcoin/issues/29839#issuecomment-2101856850)
I like the idea.
Since we are brainstorming how to get more attention to review, what about also having a tag for PRs related to projects that the core developers defined as priority for the current release cycle (`priority`)?
This will tend to attract more review effort overall, even for those more complex changes. It's not required for experienced reviewers since they can easily identify those from context. On the other hand, it might indicate to newbies like me what to pick if I would like
...
(https://github.com/bitcoin/bitcoin/issues/29839#issuecomment-2101856850)
I like the idea.
Since we are brainstorming how to get more attention to review, what about also having a tag for PRs related to projects that the core developers defined as priority for the current release cycle (`priority`)?
This will tend to attract more review effort overall, even for those more complex changes. It's not required for experienced reviewers since they can easily identify those from context. On the other hand, it might indicate to newbies like me what to pick if I would like
...
⚠️ foolbear opened an issue: "Load PSBT error: Unable to decode PSBT"
(https://github.com/bitcoin/bitcoin/issues/30070)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
"Load PSBT from ..." from menu, error shows:
`Unable to decode PSBT
Unsigned tx does not have empty scriptSigs and scriptWitnesses.: unspecified iostream_category error`
decodepsbt and analyzepsbt show the same error too:
`TX decode failed Unsigned tx does not have empty scriptSigs and scriptWitnesses.: unspecified iostream_category error (code -22)`
### Expected behaviour
Load PS
...
(https://github.com/bitcoin/bitcoin/issues/30070)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
"Load PSBT from ..." from menu, error shows:
`Unable to decode PSBT
Unsigned tx does not have empty scriptSigs and scriptWitnesses.: unspecified iostream_category error`
decodepsbt and analyzepsbt show the same error too:
`TX decode failed Unsigned tx does not have empty scriptSigs and scriptWitnesses.: unspecified iostream_category error (code -22)`
### Expected behaviour
Load PS
...
💬 ajtowns commented on pull request "crypto: add `NUMS_H` const":
(https://github.com/bitcoin/bitcoin/pull/30048#discussion_r1594997390)
nit: Would it be better to make this a `static const` member of the `XOnlyPubKey` class?
(https://github.com/bitcoin/bitcoin/pull/30048#discussion_r1594997390)
nit: Would it be better to make this a `static const` member of the `XOnlyPubKey` class?
💬 josibake commented on pull request "crypto: add `NUMS_H` const":
(https://github.com/bitcoin/bitcoin/pull/30048#discussion_r1595049543)
Can you elaborate? Not clear to me what the benefit would be.
(https://github.com/bitcoin/bitcoin/pull/30048#discussion_r1595049543)
Can you elaborate? Not clear to me what the benefit would be.
💬 laanwj commented on issue "libxcb-xinerama0 Library required by bitcoin-qt":
(https://github.com/bitcoin/bitcoin/issues/30061#issuecomment-2102081195)
> @laanwj Does https://github.com/bitcoin/bitcoin/pull/29923 address this?
Potentially it could make the dependency on xcb-xinerama optional. There's only a few functions used from that library, for a specific purpose (multiple monitor support), if the library can't be loaded, it can be assumed that this functionality isn't needed. This does mean patching Qt though.
)i've handled the xcb/wayland switch in that way--if X libraries can't be found, don't exit, but fail loading the XCB platfor
...
(https://github.com/bitcoin/bitcoin/issues/30061#issuecomment-2102081195)
> @laanwj Does https://github.com/bitcoin/bitcoin/pull/29923 address this?
Potentially it could make the dependency on xcb-xinerama optional. There's only a few functions used from that library, for a specific purpose (multiple monitor support), if the library can't be loaded, it can be assumed that this functionality isn't needed. This does mean patching Qt though.
)i've handled the xcb/wayland switch in that way--if X libraries can't be found, don't exit, but fail loading the XCB platfor
...
💬 Sjors commented on pull request "Reintroduce external signer support for Windows":
(https://github.com/bitcoin/bitcoin/pull/29868#issuecomment-2102082445)
I assume you added b69b9e85e98cac2c7585c9b613185dc6c80320cc because it no longer uses an external dependency?
Is there something you can remove from cpp-subprocess based on your comment? https://github.com/bitcoin/bitcoin/pull/29961#issuecomment-2100521158
(https://github.com/bitcoin/bitcoin/pull/29868#issuecomment-2102082445)
I assume you added b69b9e85e98cac2c7585c9b613185dc6c80320cc because it no longer uses an external dependency?
Is there something you can remove from cpp-subprocess based on your comment? https://github.com/bitcoin/bitcoin/pull/29961#issuecomment-2100521158
💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#issuecomment-2102107669)
> Would prefer this in two steps (add PCP, then remove NAT-PMP)
i'm not planning to do this, the build system commits are already set up this way, but doing it throughout would involve adding another setting in Qt just to remove it later. Same for adding a third mechanism in `portmap.cpp`. Agree with @sjors that having a forest of different port mapping settings is confusing to the user.
(https://github.com/bitcoin/bitcoin/pull/30043#issuecomment-2102107669)
> Would prefer this in two steps (add PCP, then remove NAT-PMP)
i'm not planning to do this, the build system commits are already set up this way, but doing it throughout would involve adding another setting in Qt just to remove it later. Same for adding a third mechanism in `portmap.cpp`. Agree with @sjors that having a forest of different port mapping settings is confusing to the user.
💬 Sjors commented on pull request "Testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2102131750)
@emsit wrote:
> Is there a plan to replace the path '/testnet3' with '/testnet'?
That's a mistake in the documentation, the behavior isn't changed.
@wiz wrote:
> because his node doesn't know about any yet.
Or not anymore. The seed requires some amount of recent uptime. I had one running as well, but it was on an earlier genesis block. Fixed, there goes my premine :-)
My node found your peers and synced to the tip.
> If you were able to quickly cycle the difficulty back dow
...
(https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2102131750)
@emsit wrote:
> Is there a plan to replace the path '/testnet3' with '/testnet'?
That's a mistake in the documentation, the behavior isn't changed.
@wiz wrote:
> because his node doesn't know about any yet.
Or not anymore. The seed requires some amount of recent uptime. I had one running as well, but it was on an earlier genesis block. Fixed, there goes my premine :-)
My node found your peers and synced to the tip.
> If you were able to quickly cycle the difficulty back dow
...
📝 threewebcode opened a pull request: "chore: fix some typos"
(https://github.com/bitcoin/bitcoin/pull/30071)
fix some typos
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new
...
(https://github.com/bitcoin/bitcoin/pull/30071)
fix some typos
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new
...
💬 josibake commented on pull request "crypto, refactor: add method for applying the taptweak":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1595092731)
Nice, thanks for the suggestion! This makes a ton more sense. I think it would be better to have the ctor take a pointer to the merkle root because `ApplyTapTweak` is something that you a) only want to do once over the lifetime of the object and b) will always be applied if a `merkle_root` is present (even if its `merkle_root.IsNull() == true`). I don't think this is actually a use case, but if for whatever reason you did need to do something with the key and then later apply a merkle root tweak
...
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1595092731)
Nice, thanks for the suggestion! This makes a ton more sense. I think it would be better to have the ctor take a pointer to the merkle root because `ApplyTapTweak` is something that you a) only want to do once over the lifetime of the object and b) will always be applied if a `merkle_root` is present (even if its `merkle_root.IsNull() == true`). I don't think this is actually a use case, but if for whatever reason you did need to do something with the key and then later apply a merkle root tweak
...
💬 laanwj commented on pull request "build, test, doc: Temporarily remove Android-related stuff":
(https://github.com/bitcoin/bitcoin/pull/30049#issuecomment-2102171061)
> I guess the idea is that there's basically nothing worth salvaging from our current Android build procedure?
Not sure about that. i think the idea is that the current stuff is untestable in practice, which meant the CMake transition is blocked on it. E.g. porting it as-is wouldn't result in anything usable nor testable.
i've already cautioned not to go to wild with this and throw away android compatibility in the code, but removing the user facing build system support for now, seems fin
...
(https://github.com/bitcoin/bitcoin/pull/30049#issuecomment-2102171061)
> I guess the idea is that there's basically nothing worth salvaging from our current Android build procedure?
Not sure about that. i think the idea is that the current stuff is untestable in practice, which meant the CMake transition is blocked on it. E.g. porting it as-is wouldn't result in anything usable nor testable.
i've already cautioned not to go to wild with this and throw away android compatibility in the code, but removing the user facing build system support for now, seems fin
...
💬 fanquake commented on pull request "Reintroduce external signer support for Windows":
(https://github.com/bitcoin/bitcoin/pull/29868#issuecomment-2102183930)
> I assume you added https://github.com/bitcoin/bitcoin/commit/b69b9e85e98cac2c7585c9b613185dc6c80320cc because it no longer uses an external dependency?
It's there because setting something that is already on, to on, is pointless, and doesn't test anything. In either case, external signer is also not special, so it's not clear why, as a feature, it's being explcitly enabled in the global CI config.
(https://github.com/bitcoin/bitcoin/pull/29868#issuecomment-2102183930)
> I assume you added https://github.com/bitcoin/bitcoin/commit/b69b9e85e98cac2c7585c9b613185dc6c80320cc because it no longer uses an external dependency?
It's there because setting something that is already on, to on, is pointless, and doesn't test anything. In either case, external signer is also not special, so it's not clear why, as a feature, it's being explcitly enabled in the global CI config.
💬 josibake commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#issuecomment-2102235020)
Concept ACK
Mostly focusing my review on https://github.com/bitcoin/bitcoin/pull/26606/commits/5f0cddb4f66f2a174babdfb17bc31a3011e5cc20. Regarding testing, it occurred to me that we had some large / interesting wallet files from when we were doing wallet simulations for coin selection. @murchandamus @achow101 any chance those wallets were BDB and if so, would be useful to test here?
(https://github.com/bitcoin/bitcoin/pull/26606#issuecomment-2102235020)
Concept ACK
Mostly focusing my review on https://github.com/bitcoin/bitcoin/pull/26606/commits/5f0cddb4f66f2a174babdfb17bc31a3011e5cc20. Regarding testing, it occurred to me that we had some large / interesting wallet files from when we were doing wallet simulations for coin selection. @murchandamus @achow101 any chance those wallets were BDB and if so, would be useful to test here?
💬 glozow commented on pull request "test: add MiniWallet tagging support to avoid UTXO mixing, use in `fill_mempool`":
(https://github.com/bitcoin/bitcoin/pull/29939#issuecomment-2102270164)
ACK dd8fa861939
(https://github.com/bitcoin/bitcoin/pull/29939#issuecomment-2102270164)
ACK dd8fa861939
✅ glozow closed a pull request: "chore: fix some typos"
(https://github.com/bitcoin/bitcoin/pull/30071)
(https://github.com/bitcoin/bitcoin/pull/30071)
💬 glozow commented on pull request "chore: fix some typos":
(https://github.com/bitcoin/bitcoin/pull/30071#issuecomment-2102285411)
Thanks for your interest in contributing! However, as we have hundreds of open pull requests, I am closing this to focus review on the others. (See [contributing guidelines on refactoring](https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#refactoring)).
(https://github.com/bitcoin/bitcoin/pull/30071#issuecomment-2102285411)
Thanks for your interest in contributing! However, as we have hundreds of open pull requests, I am closing this to focus review on the others. (See [contributing guidelines on refactoring](https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#refactoring)).