💬 stickies-v commented on pull request "kernel: Prune leveldb headers":
(https://github.com/bitcoin/bitcoin/pull/28186#discussion_r1283260447)
It's not immediately obvious to me why we can't stick to our usual pattern of using a `const DataStream& ssKey, CDataStream& ssValue` function signature, and resort to lambdas as is done here. I think the below code is much more straightforward, but perhaps I'm missing some nuance or drawbacks?
<details>
<summary>git diff on f26396732b940095380d76ed77685e0fb11294b8</summary>
```diff
diff --git a/src/dbwrapper.cpp b/src/dbwrapper.cpp
index 1b2548f262..cb27b739fe 100644
--- a/src/dbwrapp
...
(https://github.com/bitcoin/bitcoin/pull/28186#discussion_r1283260447)
It's not immediately obvious to me why we can't stick to our usual pattern of using a `const DataStream& ssKey, CDataStream& ssValue` function signature, and resort to lambdas as is done here. I think the below code is much more straightforward, but perhaps I'm missing some nuance or drawbacks?
<details>
<summary>git diff on f26396732b940095380d76ed77685e0fb11294b8</summary>
```diff
diff --git a/src/dbwrapper.cpp b/src/dbwrapper.cpp
index 1b2548f262..cb27b739fe 100644
--- a/src/dbwrapp
...
💬 stickies-v commented on pull request "kernel: Prune leveldb headers":
(https://github.com/bitcoin/bitcoin/pull/28186#discussion_r1283286314)
nit: I think this needs a `LIFETIMEBOUND`.
Also: suggested alternative name `DBContextRef()` to highlight that the return value can mutate our `CDBWrapper` state? No strong view on it though, happy with the current one too, just a suggestion.
(https://github.com/bitcoin/bitcoin/pull/28186#discussion_r1283286314)
nit: I think this needs a `LIFETIMEBOUND`.
Also: suggested alternative name `DBContextRef()` to highlight that the return value can mutate our `CDBWrapper` state? No strong view on it though, happy with the current one too, just a suggestion.
💬 stickies-v commented on pull request "kernel: Prune leveldb headers":
(https://github.com/bitcoin/bitcoin/pull/28186#discussion_r1283298874)
nit: Would this be an improvement, to highlight the reference nature of `db_context`, as well as I think generally it's slightly cleaner to not call `DBContext()` every time (I know it's a very cheap call)?
```suggestion
auto& db_context{DBContext()};
db_context.penv = nullptr;
```
(https://github.com/bitcoin/bitcoin/pull/28186#discussion_r1283298874)
nit: Would this be an improvement, to highlight the reference nature of `db_context`, as well as I think generally it's slightly cleaner to not call `DBContext()` every time (I know it's a very cheap call)?
```suggestion
auto& db_context{DBContext()};
db_context.penv = nullptr;
```
💬 stickies-v commented on pull request "kernel: Prune leveldb headers":
(https://github.com/bitcoin/bitcoin/pull/28186#discussion_r1283315877)
nit: would it make sense to move this to the `LevelDBContext` destructor?
(https://github.com/bitcoin/bitcoin/pull/28186#discussion_r1283315877)
nit: would it make sense to move this to the `LevelDBContext` destructor?
💬 instagibbs commented on pull request "Remove arbitrary restrictions on OP_RETURN by default":
(https://github.com/bitcoin/bitcoin/pull/28130#issuecomment-1664147208)
> This proposed change simply cleans up a bit of remaining standardness paternalism; we've gotten rid of essentially all other standardness paternalism. I can't think of any standardness mechanism other than OP_Return and first-seen (which I'm arguing to remove by default in https://github.com/bitcoin/bitcoin/pull/28132) that doesn't have a strong technical reason to exist.
I'm not going to war for this change but this strikes me as correct. I classified the OP_RETURN limit as the last remain
...
(https://github.com/bitcoin/bitcoin/pull/28130#issuecomment-1664147208)
> This proposed change simply cleans up a bit of remaining standardness paternalism; we've gotten rid of essentially all other standardness paternalism. I can't think of any standardness mechanism other than OP_Return and first-seen (which I'm arguing to remove by default in https://github.com/bitcoin/bitcoin/pull/28132) that doesn't have a strong technical reason to exist.
I'm not going to war for this change but this strikes me as correct. I classified the OP_RETURN limit as the last remain
...
💬 RobinLinus commented on pull request "Remove arbitrary restrictions on OP_RETURN by default":
(https://github.com/bitcoin/bitcoin/pull/28130#issuecomment-1664150253)
> Why do you believe that expanding a publication mechanism that is 4x more expensive than the heavily used taproot witness mechanism will "increase the spam problems significantly"?
@ChristopherA just said that he's paying you to push this because it makes it cheaper and easier for him to write his non-bitcoin stuff into the chain.
(https://github.com/bitcoin/bitcoin/pull/28130#issuecomment-1664150253)
> Why do you believe that expanding a publication mechanism that is 4x more expensive than the heavily used taproot witness mechanism will "increase the spam problems significantly"?
@ChristopherA just said that he's paying you to push this because it makes it cheaper and easier for him to write his non-bitcoin stuff into the chain.
💬 pinheadmz commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1283345857)
OK I see. In that case, it can be a follow-up. Probably there is some reason we keep the scheduler and don't resend right on startup (we wouldn't want spy nodes detecting our restart! ...?)
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1283345857)
OK I see. In that case, it can be a follow-up. Probably there is some reason we keep the scheduler and don't resend right on startup (we wouldn't want spy nodes detecting our restart! ...?)
💬 theuni commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1283379722)
FWIW, I made this non-default because I don't think it's a good idea for a vanilla `make` to spew warnings _on purpose_. It's bound to set off c-i somewhere eventually.
As @MarcoFalke points out though, we do want it run when we're in control of the environment and not worried about those warnings. In order to run them, I suggest:
```diff
diff --git a/ci/test/06_script_b.sh b/ci/test/06_script_b.sh
index 75d5469267..02358db789 100755
--- a/ci/test/06_script_b.sh
+++ b/ci/test/06_script_b
...
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1283379722)
FWIW, I made this non-default because I don't think it's a good idea for a vanilla `make` to spew warnings _on purpose_. It's bound to set off c-i somewhere eventually.
As @MarcoFalke points out though, we do want it run when we're in control of the environment and not worried about those warnings. In order to run them, I suggest:
```diff
diff --git a/ci/test/06_script_b.sh b/ci/test/06_script_b.sh
index 75d5469267..02358db789 100755
--- a/ci/test/06_script_b.sh
+++ b/ci/test/06_script_b
...
💬 pinheadmz commented on pull request "test: refactor: deduplicate segwitv0 ECDSA signing for tx inputs":
(https://github.com/bitcoin/bitcoin/pull/28154#discussion_r1283381371)
I kinda feel like you don't need to use the helper for this one. It's actually +2 lines as opposed to -2 in the other locations.
(https://github.com/bitcoin/bitcoin/pull/28154#discussion_r1283381371)
I kinda feel like you don't need to use the helper for this one. It's actually +2 lines as opposed to -2 in the other locations.
🤔 pinheadmz reviewed a pull request: "test: refactor: deduplicate segwitv0 ECDSA signing for tx inputs"
(https://github.com/bitcoin/bitcoin/pull/28154#pullrequestreview-1561373322)
code review ACK at 83d7cfd5429b0be7755bc48145032956f5f56dae
(https://github.com/bitcoin/bitcoin/pull/28154#pullrequestreview-1561373322)
code review ACK at 83d7cfd5429b0be7755bc48145032956f5f56dae
💬 theuni commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#issuecomment-1664210830)
@fanquake Would you mind removing the single use of `NOLINT` and pushing, just so that we can make sure the c-i still fails as expected after all the changes?
Assuming it does, LGTM either with or without addressing [this comment](https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1279354069).
(https://github.com/bitcoin/bitcoin/pull/26296#issuecomment-1664210830)
@fanquake Would you mind removing the single use of `NOLINT` and pushing, just so that we can make sure the c-i still fails as expected after all the changes?
Assuming it does, LGTM either with or without addressing [this comment](https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1279354069).
💬 theuni commented on pull request "refactor: Remove unused includes from wallet.cpp":
(https://github.com/bitcoin/bitcoin/pull/28200#issuecomment-1664234300)
Concept ACK. I didn't review the includes changes in detail, but the move makes sense to me. And I'm always up for one-data-structure-per-header :)
(https://github.com/bitcoin/bitcoin/pull/28200#issuecomment-1664234300)
Concept ACK. I didn't review the includes changes in detail, but the move makes sense to me. And I'm always up for one-data-structure-per-header :)
💬 fanquake commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1283423591)
Added in the current push.
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1283423591)
Added in the current push.
💬 fanquake commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#issuecomment-1664258585)
> Would you mind removing the single use of NOLINT and pushing, just so that we can make sure the c-i still fails as expected after all the changes?
Done.
(https://github.com/bitcoin/bitcoin/pull/26296#issuecomment-1664258585)
> Would you mind removing the single use of NOLINT and pushing, just so that we can make sure the c-i still fails as expected after all the changes?
Done.
👍 instagibbs approved a pull request: "util: Type-safe transaction identifiers"
(https://github.com/bitcoin/bitcoin/pull/28107#pullrequestreview-1555428522)
ACK 76fe2c304201646d63b1b01b397e18a99252a2d9
played around with some extensions to see where it could go, I think longer term we could use `GenTxid` for situations where both are acceptable, and `Wtxid/Txid` when only one type is, and slowly migrate towards that. f.e. `GenTxid` could have a `GetTypedHash` to convert to the other.
(https://github.com/bitcoin/bitcoin/pull/28107#pullrequestreview-1555428522)
ACK 76fe2c304201646d63b1b01b397e18a99252a2d9
played around with some extensions to see where it could go, I think longer term we could use `GenTxid` for situations where both are acceptable, and `Wtxid/Txid` when only one type is, and slowly migrate towards that. f.e. `GenTxid` could have a `GetTypedHash` to convert to the other.
💬 instagibbs commented on pull request "util: Type-safe transaction identifiers":
(https://github.com/bitcoin/bitcoin/pull/28107#discussion_r1279668749)
Let's use `Txid` here as well
(https://github.com/bitcoin/bitcoin/pull/28107#discussion_r1279668749)
Let's use `Txid` here as well
💬 TheCharlatan commented on pull request "kernel: Prune leveldb headers":
(https://github.com/bitcoin/bitcoin/pull/28186#discussion_r1283434519)
I originally did this to keep the stream reading within the try catch block, while changing as few lines in the implementation as possible. We could just return the string value though in the `ReadImpl` and then complete the data reading in the `Read` function.
(https://github.com/bitcoin/bitcoin/pull/28186#discussion_r1283434519)
I originally did this to keep the stream reading within the try catch block, while changing as few lines in the implementation as possible. We could just return the string value though in the `ReadImpl` and then complete the data reading in the `Read` function.
📝 eriknylund opened a pull request: "test: verify spend from 999-of-999 taproot multisig wallet"
(https://github.com/bitcoin/bitcoin/pull/28212)
Verify that a 999-of-999 taproot multisig wallet is possible and can spend from it, and that neither a 1-of-1000 nor 999-of-1000 is allowed.
The tests will require some time to run. On my Mac M1 the new tests run in about 40 seconds.
Had to bump fee rate to resolve assertions in sendtoaddress and psbt methods:
- code -5: assert rpc_online.gettransaction(txid)["confirmations"] > 0
- code -26: min relay fee not met
Fixes #28179
(https://github.com/bitcoin/bitcoin/pull/28212)
Verify that a 999-of-999 taproot multisig wallet is possible and can spend from it, and that neither a 1-of-1000 nor 999-of-1000 is allowed.
The tests will require some time to run. On my Mac M1 the new tests run in about 40 seconds.
Had to bump fee rate to resolve assertions in sendtoaddress and psbt methods:
- code -5: assert rpc_online.gettransaction(txid)["confirmations"] > 0
- code -26: min relay fee not met
Fixes #28179
💬 achow101 commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#issuecomment-1664282446)
> But it could produce a corrupt dump, which someone might rely on for their backup (even if they shouldn't).
I've changed it to be optional and enabled with the argument `-withinternalbdb`.
> Not sure if that was fixed in your later push, but in any case,
Yes, I fixed that
> I pushed another crash to the qa-assets PR.
This seems to be crash in BDB itself rather than in our parser.
(https://github.com/bitcoin/bitcoin/pull/26606#issuecomment-1664282446)
> But it could produce a corrupt dump, which someone might rely on for their backup (even if they shouldn't).
I've changed it to be optional and enabled with the argument `-withinternalbdb`.
> Not sure if that was fixed in your later push, but in any case,
Yes, I fixed that
> I pushed another crash to the qa-assets PR.
This seems to be crash in BDB itself rather than in our parser.
💬 fanquake commented on pull request "[WIP] guix: use `-muse-unaligned-vector-move` for Windows builds":
(https://github.com/bitcoin/bitcoin/pull/28151#issuecomment-1664282498)
> Any reason not to add it to our configure directly rather than guix?
I think that's actually all we can do here, adding this (to configure) as a best-effort for anyone using an unpatched compiler (where it'd be most beneficial when building `bitcoind.exe`), while also retaining our GCC patching in Guix. Anything else would potentially leave us with unwanted code from dependencies/the rest of the toolchain.
(https://github.com/bitcoin/bitcoin/pull/28151#issuecomment-1664282498)
> Any reason not to add it to our configure directly rather than guix?
I think that's actually all we can do here, adding this (to configure) as a best-effort for anyone using an unpatched compiler (where it'd be most beneficial when building `bitcoind.exe`), while also retaining our GCC patching in Guix. Anything else would potentially leave us with unwanted code from dependencies/the rest of the toolchain.