💬 brunoerg commented on pull request "Improve display address handling for external signer":
(https://github.com/bitcoin/bitcoin/pull/24313#issuecomment-1867779809)
> @achow101 any thoughts on having HWI catch this cryptic 0x6982 status code and throw a more clear error (that we can then just pass on to the user)?
Not sure, but this status code is specific from Ledger?
(https://github.com/bitcoin/bitcoin/pull/24313#issuecomment-1867779809)
> @achow101 any thoughts on having HWI catch this cryptic 0x6982 status code and throw a more clear error (that we can then just pass on to the user)?
Not sure, but this status code is specific from Ledger?
💬 theuni commented on issue "Unnecessary symbols being exported in `libbitcoinconsensus.so`":
(https://github.com/bitcoin/bitcoin/issues/26698#issuecomment-1867794427)
Adding one more bit to complicate and maybe explain why I described the boundary as a firewall... We build `libbitcoinconsensus.so` with guix using `-static-libstdc++` (side-note, we should perhaps move this into the build-system rather than guix).
So the idea here is that the entire c++ implementation should be compiled into the shared lib with no unresolved c++ stdlib symbols, and exporting only C symbols. That way the end-user can use any c++ impl (if any) they want without fear of collisi
...
(https://github.com/bitcoin/bitcoin/issues/26698#issuecomment-1867794427)
Adding one more bit to complicate and maybe explain why I described the boundary as a firewall... We build `libbitcoinconsensus.so` with guix using `-static-libstdc++` (side-note, we should perhaps move this into the build-system rather than guix).
So the idea here is that the entire c++ implementation should be compiled into the shared lib with no unresolved c++ stdlib symbols, and exporting only C symbols. That way the end-user can use any c++ impl (if any) they want without fear of collisi
...
💬 hebasto commented on pull request "build: Build `libbitcoinconsensus` from its own convenience library":
(https://github.com/bitcoin/bitcoin/pull/24994#issuecomment-1867811444)
> I'd like to help with this, but I just don't understand most of the discussion so far.
My apologies about that.
> I see that this fixes some kind of a bug on windows and is supposed to have other benefits mentioned in the PR description, but I don't see a clear description of the bug anywhere.
https://github.com/bitcoin/bitcoin/pull/24994#issue-1216293178:
> This PR prunes all of [unnecessary](https://github.com/bitcoin/bitcoin/pull/25020#issuecomment-1142151675) symbols being expor
...
(https://github.com/bitcoin/bitcoin/pull/24994#issuecomment-1867811444)
> I'd like to help with this, but I just don't understand most of the discussion so far.
My apologies about that.
> I see that this fixes some kind of a bug on windows and is supposed to have other benefits mentioned in the PR description, but I don't see a clear description of the bug anywhere.
https://github.com/bitcoin/bitcoin/pull/24994#issue-1216293178:
> This PR prunes all of [unnecessary](https://github.com/bitcoin/bitcoin/pull/25020#issuecomment-1142151675) symbols being expor
...
💬 sipa commented on pull request "util: Faster std::byte (pre)vector (un)serialize":
(https://github.com/bitcoin/bitcoin/pull/29114#issuecomment-1867811476)
utACK fab41697a5448ef2861f65795bd63a4ccdda6a40
(https://github.com/bitcoin/bitcoin/pull/29114#issuecomment-1867811476)
utACK fab41697a5448ef2861f65795bd63a4ccdda6a40
💬 sr-gi commented on pull request "test: adds outbound eviction functional tests, updates comment in ConsiderEviction":
(https://github.com/bitcoin/bitcoin/pull/29122#discussion_r1435164703)
Looks like I'm not the only one who took a minute to get this 😅
I wouldn't mind expanding the comment if there is am overall agreement that it is incomplete
(https://github.com/bitcoin/bitcoin/pull/29122#discussion_r1435164703)
Looks like I'm not the only one who took a minute to get this 😅
I wouldn't mind expanding the comment if there is am overall agreement that it is incomplete
💬 Sjors commented on pull request "Improve display address handling for external signer":
(https://github.com/bitcoin/bitcoin/pull/24313#issuecomment-1867824658)
@brunoerg most likely yes, but I can't find a full list anywhere.
(https://github.com/bitcoin/bitcoin/pull/24313#issuecomment-1867824658)
@brunoerg most likely yes, but I can't find a full list anywhere.
📝 maflcko opened a pull request: " refactor: Allow std::span construction from CKey "
(https://github.com/bitcoin/bitcoin/pull/29133)
Is is possible to construct a `Span` from a reference to a `CKey`. However, the same is not possible with `std::span`.
Fix that, and add a commit to verify the fix.
(https://github.com/bitcoin/bitcoin/pull/29133)
Is is possible to construct a `Span` from a reference to a `CKey`. However, the same is not possible with `std::span`.
Fix that, and add a commit to verify the fix.
💬 brunoerg commented on pull request "Improve display address handling for external signer":
(https://github.com/bitcoin/bitcoin/pull/24313#issuecomment-1867855528)
> @brunoerg most likely yes, but I can't find a full list anywhere.
Me either. I've some of them but I manually mapped.
(https://github.com/bitcoin/bitcoin/pull/24313#issuecomment-1867855528)
> @brunoerg most likely yes, but I can't find a full list anywhere.
Me either. I've some of them but I manually mapped.
💬 fanquake commented on issue "Unnecessary symbols being exported in `libbitcoinconsensus.so`":
(https://github.com/bitcoin/bitcoin/issues/26698#issuecomment-1867858105)
> Adding one more bit to complicate
Thanks, I'll pile on some more. I've alluded to this in other threads (#24994), but still haven't seen a summary of:
* What currently happens with `lld` + `libstdc++`?
* What currently happens with `lld` + `libc++`?
* Maybe even, what happens with [`mold`](https://github.com/rui314/mold) and either standard library?
* What happens under LTO, in any combination of the above?
There is much more to consider here than "What does `ld` happen to do", and t
...
(https://github.com/bitcoin/bitcoin/issues/26698#issuecomment-1867858105)
> Adding one more bit to complicate
Thanks, I'll pile on some more. I've alluded to this in other threads (#24994), but still haven't seen a summary of:
* What currently happens with `lld` + `libstdc++`?
* What currently happens with `lld` + `libc++`?
* Maybe even, what happens with [`mold`](https://github.com/rui314/mold) and either standard library?
* What happens under LTO, in any combination of the above?
There is much more to consider here than "What does `ld` happen to do", and t
...
💬 mzumsande commented on pull request "validation: improve performance of CheckBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/28339#issuecomment-1867862091)
I added a commit with a benchmark, and another one that allow to specify the frequency for `-checkblockindex` (analogous to the way `-checkaddrman` and `-checkmempool` work).
> Another optimization that might be worth it, it is to replace the std::multimap<CBlockIndex*,CBlockIndex*> forward with an std::unordered_map<CBlockIndex*, std::vector<CBlockIndex*>> forward.
I tried that in `https://github.com/mzumsande/bitcoin/commit/51aae54cfafc9700581e69dc129866cff76925ae` (directly on master wi
...
(https://github.com/bitcoin/bitcoin/pull/28339#issuecomment-1867862091)
I added a commit with a benchmark, and another one that allow to specify the frequency for `-checkblockindex` (analogous to the way `-checkaddrman` and `-checkmempool` work).
> Another optimization that might be worth it, it is to replace the std::multimap<CBlockIndex*,CBlockIndex*> forward with an std::unordered_map<CBlockIndex*, std::vector<CBlockIndex*>> forward.
I tried that in `https://github.com/mzumsande/bitcoin/commit/51aae54cfafc9700581e69dc129866cff76925ae` (directly on master wi
...
💬 sipa commented on pull request "lib: add taproot support to libconsensus":
(https://github.com/bitcoin/bitcoin/pull/28539#issuecomment-1867869031)
@panicfarm libconsensus had certainly contained all the logic for taproot validation ever since Bitcoin Core had it, but it was not exposed through the library interface, for two reasons:
* The API requires passing in flags to select which softfork rules to apply to validation. No such flag was exposed for taproot.
* Taproot validation requires access to more information that wasn't present in the libconsensus API (specifically, the UTXO data of all spent UTXOs by a transaction). New functions
...
(https://github.com/bitcoin/bitcoin/pull/28539#issuecomment-1867869031)
@panicfarm libconsensus had certainly contained all the logic for taproot validation ever since Bitcoin Core had it, but it was not exposed through the library interface, for two reasons:
* The API requires passing in flags to select which softfork rules to apply to validation. No such flag was exposed for taproot.
* Taproot validation requires access to more information that wasn't present in the libconsensus API (specifically, the UTXO data of all spent UTXOs by a transaction). New functions
...
💬 hebasto commented on issue "Unnecessary symbols being exported in `libbitcoinconsensus.so`":
(https://github.com/bitcoin/bitcoin/issues/26698#issuecomment-1867869356)
Our current release process uses a very particular toolchain. While LTO might be considered as its update in the near future, it is not clear how other combinations of tools are related to this problem? Unless we would prefer to alter the release toolchain instead of moving some code into a separated translation unit to fix the shared library symbol visibility.
(https://github.com/bitcoin/bitcoin/issues/26698#issuecomment-1867869356)
Our current release process uses a very particular toolchain. While LTO might be considered as its update in the near future, it is not clear how other combinations of tools are related to this problem? Unless we would prefer to alter the release toolchain instead of moving some code into a separated translation unit to fix the shared library symbol visibility.
💬 ryanofsky commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#issuecomment-1867873246)
Thanks, I was also trying to think of names for these other than void descriptors. For example maybe they could be called data descriptors and written like data(xpub...) to convey the idea that these descriptors are just used to hold data and don't produce scriptpubkeys.
(https://github.com/bitcoin/bitcoin/pull/26728#issuecomment-1867873246)
Thanks, I was also trying to think of names for these other than void descriptors. For example maybe they could be called data descriptors and written like data(xpub...) to convey the idea that these descriptors are just used to hold data and don't produce scriptpubkeys.
💬 fanquake commented on issue "Unnecessary symbols being exported in `libbitcoinconsensus.so`":
(https://github.com/bitcoin/bitcoin/issues/26698#issuecomment-1867877948)
> Our current release process
This isn't specific to the release process. Anyone can, and should expect to be able to build "working" libs/bins outside of Guix.
> it is not clear how other combinations of tools are related to this problem?
If you "fix' anything by doing something that isn't supported by those tools, i.e use a special link/compile flag, or rely on ld or libc++ specific behaviors, you're going to break things/be incompatible with anyone using any other tools. Also, any ch
...
(https://github.com/bitcoin/bitcoin/issues/26698#issuecomment-1867877948)
> Our current release process
This isn't specific to the release process. Anyone can, and should expect to be able to build "working" libs/bins outside of Guix.
> it is not clear how other combinations of tools are related to this problem?
If you "fix' anything by doing something that isn't supported by those tools, i.e use a special link/compile flag, or rely on ld or libc++ specific behaviors, you're going to break things/be incompatible with anyone using any other tools. Also, any ch
...
💬 mzumsande commented on pull request "validation: improve performance of CheckBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/28339#issuecomment-1867883982)
Benchmark results:
<details>
<summary>master</summary>
| ns/op | op/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 331,165.67 | 3,019.64 | 0.3% | 0.01 | `CheckBlockIndex`
| 320,603.00 | 3,119.12 | 1.7% | 0.01 | `CheckBlockIndex`
| 315,683.67 | 3,167.73 | 0.4% | 0.01 | `CheckBlockIndex`
| 329,077.3
...
(https://github.com/bitcoin/bitcoin/pull/28339#issuecomment-1867883982)
Benchmark results:
<details>
<summary>master</summary>
| ns/op | op/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 331,165.67 | 3,019.64 | 0.3% | 0.01 | `CheckBlockIndex`
| 320,603.00 | 3,119.12 | 1.7% | 0.01 | `CheckBlockIndex`
| 315,683.67 | 3,167.73 | 0.4% | 0.01 | `CheckBlockIndex`
| 329,077.3
...
👋 mzumsande's pull request is ready for review: "validation: improve performance of CheckBlockIndex"
(https://github.com/bitcoin/bitcoin/pull/28339)
(https://github.com/bitcoin/bitcoin/pull/28339)
💬 hebasto commented on issue "Unnecessary symbols being exported in `libbitcoinconsensus.so`":
(https://github.com/bitcoin/bitcoin/issues/26698#issuecomment-1867886871)
> If you "fix' anything by doing something that isn't supported by those tools, i.e use a special link/compile flag, or rely on ld or libc++ specific behaviors, you're going to break things/be incompatible with anyone using any other tools.
Right. Requiring that a fix does not introduce any regressions is obvious.
By the way, there were no comments regarding any introduced break in https://github.com/bitcoin/bitcoin/pull/24994.
I mean, what is the point to analyze the current behavior wit
...
(https://github.com/bitcoin/bitcoin/issues/26698#issuecomment-1867886871)
> If you "fix' anything by doing something that isn't supported by those tools, i.e use a special link/compile flag, or rely on ld or libc++ specific behaviors, you're going to break things/be incompatible with anyone using any other tools.
Right. Requiring that a fix does not introduce any regressions is obvious.
By the way, there were no comments regarding any introduced break in https://github.com/bitcoin/bitcoin/pull/24994.
I mean, what is the point to analyze the current behavior wit
...
💬 achow101 commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#issuecomment-1867889571)
> This is not actually true. Thinking about this more, I wonder if a more natural way of representing standalone keys in the wallet is just to treat them as a new [descriptor type](https://github.com/bitcoin/bitcoin/blob/master/doc/descriptors.md#reference), like `void(KEY)`, where the void descriptor would hold the key, but instead of [expanding](https://github.com/bitcoin/bitcoin/blob/4b1196a9855dcd188a24f393aa2fa21e2d61f061/src/script/descriptor.h#L98) to a scriptPubKey or set of scriptPubKey
...
(https://github.com/bitcoin/bitcoin/pull/26728#issuecomment-1867889571)
> This is not actually true. Thinking about this more, I wonder if a more natural way of representing standalone keys in the wallet is just to treat them as a new [descriptor type](https://github.com/bitcoin/bitcoin/blob/master/doc/descriptors.md#reference), like `void(KEY)`, where the void descriptor would hold the key, but instead of [expanding](https://github.com/bitcoin/bitcoin/blob/4b1196a9855dcd188a24f393aa2fa21e2d61f061/src/script/descriptor.h#L98) to a scriptPubKey or set of scriptPubKey
...
🤔 murchandamus reviewed a pull request: "wallet: Add CoinGrinder coin selection algorithm"
(https://github.com/bitcoin/bitcoin/pull/27877#pullrequestreview-1794686545)
Rebased and addressed comments by @kashifs. Thanks for the thorough read, @kashifs.
(https://github.com/bitcoin/bitcoin/pull/27877#pullrequestreview-1794686545)
Rebased and addressed comments by @kashifs. Thanks for the thorough read, @kashifs.
💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1435191286)
Thanks, I’ll fix that
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1435191286)
Thanks, I’ll fix that
💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1435192987)
Thanks, I’ve amended the documentation
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1435192987)
Thanks, I’ve amended the documentation