💬 maflcko commented on pull request "build: Fix check whether `-latomic` needed":
(https://github.com/bitcoin/bitcoin/pull/29177#issuecomment-1877033482)
Testing on a fresh install of Ubuntu 24.04 Noble: `export DEBIAN_FRONTEND=noninteractive && apt update && apt install curl wget htop git vim ccache -y && git clone https://github.com/bitcoin/bitcoin.git --depth=1 && cd bitcoin && git fetch origin --depth=1 f8ca1357c8205ceff732dcfb0d2bad79b40b611b && git checkout f8ca1357c8205ceff732dcfb0d2bad79b40b611b && apt install build-essential libtool autotools-dev automake pkg-config bsdmainutils python3 make automake cmake curl clang-17 llvm-17 g++-multi
...
(https://github.com/bitcoin/bitcoin/pull/29177#issuecomment-1877033482)
Testing on a fresh install of Ubuntu 24.04 Noble: `export DEBIAN_FRONTEND=noninteractive && apt update && apt install curl wget htop git vim ccache -y && git clone https://github.com/bitcoin/bitcoin.git --depth=1 && cd bitcoin && git fetch origin --depth=1 f8ca1357c8205ceff732dcfb0d2bad79b40b611b && git checkout f8ca1357c8205ceff732dcfb0d2bad79b40b611b && apt install build-essential libtool autotools-dev automake pkg-config bsdmainutils python3 make automake cmake curl clang-17 llvm-17 g++-multi
...
💬 maflcko commented on pull request "build: Fix check whether `-latomic` needed":
(https://github.com/bitcoin/bitcoin/pull/29177#issuecomment-1877033681)
lgtm ACK f8ca1357c8205ceff732dcfb0d2bad79b40b611b
(https://github.com/bitcoin/bitcoin/pull/29177#issuecomment-1877033681)
lgtm ACK f8ca1357c8205ceff732dcfb0d2bad79b40b611b
💬 michaelfolkson commented on pull request "datacarriersize: Match more datacarrying":
(https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1877045032)
Concept NACK.
1) The high fee rate, consensus valid transactions that Ocean mines is Ocean's business but trying to prevent high fee rate, consensus valid transactions from finding their way to **all** miners is a losing game. Even if this PR was merged the transactions could be submitted to miners out of band.
2) Transactions are just bytes with constraints imposed on them by the consensus rules. There is an endless list of ways small chunks of data (ie bytes) can be embedded in consensu
...
(https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1877045032)
Concept NACK.
1) The high fee rate, consensus valid transactions that Ocean mines is Ocean's business but trying to prevent high fee rate, consensus valid transactions from finding their way to **all** miners is a losing game. Even if this PR was merged the transactions could be submitted to miners out of band.
2) Transactions are just bytes with constraints imposed on them by the consensus rules. There is an endless list of ways small chunks of data (ie bytes) can be embedded in consensu
...
💬 ajtowns commented on pull request "doc: revert clarify -datacarriersize":
(https://github.com/bitcoin/bitcoin/pull/29173#issuecomment-1877052212)
> You never change software requirements to fit the functionality.
The help text and documentation is not generally a statement of requirements, rather it documents what the software does, whether that's optimal/desirable or not. The closest thing we have to requirements docs are the BIPs, however the senior editor there [considers node policies](https://github.com/bitcoin/bips/pull/1524#issuecomment-1869734387) like this as being out of scope there as well.
(https://github.com/bitcoin/bitcoin/pull/29173#issuecomment-1877052212)
> You never change software requirements to fit the functionality.
The help text and documentation is not generally a statement of requirements, rather it documents what the software does, whether that's optimal/desirable or not. The closest thing we have to requirements docs are the BIPs, however the senior editor there [considers node policies](https://github.com/bitcoin/bips/pull/1524#issuecomment-1869734387) like this as being out of scope there as well.
💬 sipa commented on pull request "Update libsecp256k1 subtree for 0.4.1 release":
(https://github.com/bitcoin/bitcoin/pull/29169#issuecomment-1877091223)
> What do you mean by/where are we reporting the version number?
The commit title, "Update secp256k1 subtree to upstream release 0.4.1", makes it sound like it's updating to the exact 0.4.1 tag. I agree it's fine to update to master, and that we shouldn't need a policy of only using tagged releases, but it'd be better if the commit title said "Update secp256k1 subtree to current master", or "to commit after 0.4.1".
(https://github.com/bitcoin/bitcoin/pull/29169#issuecomment-1877091223)
> What do you mean by/where are we reporting the version number?
The commit title, "Update secp256k1 subtree to upstream release 0.4.1", makes it sound like it's updating to the exact 0.4.1 tag. I agree it's fine to update to master, and that we shouldn't need a policy of only using tagged releases, but it'd be better if the commit title said "Update secp256k1 subtree to current master", or "to commit after 0.4.1".
💬 furszy commented on pull request "sqlite: Disallow writing from multiple `SQLiteBatch`s":
(https://github.com/bitcoin/bitcoin/pull/29112#discussion_r1441747639)
This will fail to compile without sqlite support. Need to scope the test under the `USE_SQLITE` compile flag.
```diff
diff --git a/src/wallet/test/db_tests.cpp b/src/wallet/test/db_tests.cpp
--- a/src/wallet/test/db_tests.cpp (revision f1b5b259f6d4759e7fc514b82b4096f7669e8325)
+++ b/src/wallet/test/db_tests.cpp (date 1704374489489)
@@ -228,6 +228,7 @@
}
}
+#ifdef USE_SQLITE
BOOST_AUTO_TEST_CASE(concurrent_txn_dont_interfere)
{
std::string key = "key";
@@ -269,6 +270,
...
(https://github.com/bitcoin/bitcoin/pull/29112#discussion_r1441747639)
This will fail to compile without sqlite support. Need to scope the test under the `USE_SQLITE` compile flag.
```diff
diff --git a/src/wallet/test/db_tests.cpp b/src/wallet/test/db_tests.cpp
--- a/src/wallet/test/db_tests.cpp (revision f1b5b259f6d4759e7fc514b82b4096f7669e8325)
+++ b/src/wallet/test/db_tests.cpp (date 1704374489489)
@@ -228,6 +228,7 @@
}
}
+#ifdef USE_SQLITE
BOOST_AUTO_TEST_CASE(concurrent_txn_dont_interfere)
{
std::string key = "key";
@@ -269,6 +270,
...
💬 brunoerg commented on pull request "fuzz: a target for the block index database":
(https://github.com/bitcoin/bitcoin/pull/28209#discussion_r1441757749)
nit (feel free to ignore):
```diff
diff --git a/src/test/fuzz/block_index.cpp b/src/test/fuzz/block_index.cpp
index 7885eda747..3f2a55f310 100644
--- a/src/test/fuzz/block_index.cpp
+++ b/src/test/fuzz/block_index.cpp
@@ -54,11 +54,6 @@ void init_block_index()
FUZZ_TARGET(block_index, .init = init_block_index)
{
FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()};
- auto block_index = kernel::BlockTreeDB(DBParams{
- .path = "", // Memory only.
-
...
(https://github.com/bitcoin/bitcoin/pull/28209#discussion_r1441757749)
nit (feel free to ignore):
```diff
diff --git a/src/test/fuzz/block_index.cpp b/src/test/fuzz/block_index.cpp
index 7885eda747..3f2a55f310 100644
--- a/src/test/fuzz/block_index.cpp
+++ b/src/test/fuzz/block_index.cpp
@@ -54,11 +54,6 @@ void init_block_index()
FUZZ_TARGET(block_index, .init = init_block_index)
{
FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()};
- auto block_index = kernel::BlockTreeDB(DBParams{
- .path = "", // Memory only.
-
...
💬 brunoerg commented on pull request "fuzz: a target for the block index database":
(https://github.com/bitcoin/bitcoin/pull/28209#discussion_r1441762014)
nit (feel free to ignore):
```diff
diff --git a/src/test/fuzz/block_index.cpp b/src/test/fuzz/block_index.cpp
index 7885eda747..bc073e4930 100644
--- a/src/test/fuzz/block_index.cpp
+++ b/src/test/fuzz/block_index.cpp
@@ -93,8 +93,8 @@ FUZZ_TARGET(block_index, .init = init_block_index)
// We should be able to read every block file info we stored. Its value should correspond to
// what we stored above.
- CBlockFileInfo info;
for (const auto& [n, file_info]: files_
...
(https://github.com/bitcoin/bitcoin/pull/28209#discussion_r1441762014)
nit (feel free to ignore):
```diff
diff --git a/src/test/fuzz/block_index.cpp b/src/test/fuzz/block_index.cpp
index 7885eda747..bc073e4930 100644
--- a/src/test/fuzz/block_index.cpp
+++ b/src/test/fuzz/block_index.cpp
@@ -93,8 +93,8 @@ FUZZ_TARGET(block_index, .init = init_block_index)
// We should be able to read every block file info we stored. Its value should correspond to
// what we stored above.
- CBlockFileInfo info;
for (const auto& [n, file_info]: files_
...
👍 brunoerg approved a pull request: "fuzz: a target for the block index database"
(https://github.com/bitcoin/bitcoin/pull/28209#pullrequestreview-1804183397)
crACK 8083aa21a699cb240a11741be8ba5cfeeb58ee8b
left some nits
(https://github.com/bitcoin/bitcoin/pull/28209#pullrequestreview-1804183397)
crACK 8083aa21a699cb240a11741be8ba5cfeeb58ee8b
left some nits
🤔 furszy reviewed a pull request: "sqlite: Disallow writing from multiple `SQLiteBatch`s"
(https://github.com/bitcoin/bitcoin/pull/29112#pullrequestreview-1804185742)
> I don't think this particular issue is a problem for us however. The fact that it deadlocks in this particular test case is probably a good thing since it means that BDB is handling locking internally. If there were two threads trying to read/write at the same time, BDB would force one thread to wait for the other, as this PR is doing for SQLite. That's the behavior that we want. It's only a deadlock (and test failure) since the test is trying to do the behavior of two threads within one threa
...
(https://github.com/bitcoin/bitcoin/pull/29112#pullrequestreview-1804185742)
> I don't think this particular issue is a problem for us however. The fact that it deadlocks in this particular test case is probably a good thing since it means that BDB is handling locking internally. If there were two threads trying to read/write at the same time, BDB would force one thread to wait for the other, as this PR is doing for SQLite. That's the behavior that we want. It's only a deadlock (and test failure) since the test is trying to do the behavior of two threads within one threa
...
💬 josibake commented on pull request "wallet, rpc: `FundTransaction` refactor":
(https://github.com/bitcoin/bitcoin/pull/28560#discussion_r1441785861)
noted! will update
(https://github.com/bitcoin/bitcoin/pull/28560#discussion_r1441785861)
noted! will update
💬 josibake commented on pull request "wallet, rpc: `FundTransaction` refactor":
(https://github.com/bitcoin/bitcoin/pull/28560#discussion_r1441789986)
I initially started putting these tests in `wallet_basic.py`, but I'd prefer if we have RPC-specific test files and move all of the `sendmany` specific tests into the new `wallet_sendmany.py` file in a follow-up PR.
(https://github.com/bitcoin/bitcoin/pull/28560#discussion_r1441789986)
I initially started putting these tests in `wallet_basic.py`, but I'd prefer if we have RPC-specific test files and move all of the `sendmany` specific tests into the new `wallet_sendmany.py` file in a follow-up PR.
💬 darosior commented on pull request "fuzz: a target for the block index database":
(https://github.com/bitcoin/bitcoin/pull/28209#discussion_r1441791286)
Why?
(https://github.com/bitcoin/bitcoin/pull/28209#discussion_r1441791286)
Why?
💬 real-or-random commented on pull request "Update libsecp256k1 subtree for 0.4.1 release":
(https://github.com/bitcoin/bitcoin/pull/29169#issuecomment-1877165550)
> What do you mean by/where are we reporting the version number?
Plus, `./configure --help` in the subtree mentions `libsecp256k1 0.4.2-dev [...]`, but that's really a minor thing.
(https://github.com/bitcoin/bitcoin/pull/29169#issuecomment-1877165550)
> What do you mean by/where are we reporting the version number?
Plus, `./configure --help` in the subtree mentions `libsecp256k1 0.4.2-dev [...]`, but that's really a minor thing.
💬 maflcko commented on issue "ubsan: misaligned-pointer-use in crc32c/src/crc32c_arm64.cc":
(https://github.com/bitcoin/bitcoin/issues/29178#issuecomment-1877178197)
Same with `./ci/test/00_setup_env_native_asan.sh`.
I somehow assumed that this was fixed by:
* https://github.com/bitcoin/bitcoin/pull/27360
* https://github.com/bitcoin/bitcoin/pull/27298
* https://github.com/bitcoin/bitcoin/pull/27363
(https://github.com/bitcoin/bitcoin/issues/29178#issuecomment-1877178197)
Same with `./ci/test/00_setup_env_native_asan.sh`.
I somehow assumed that this was fixed by:
* https://github.com/bitcoin/bitcoin/pull/27360
* https://github.com/bitcoin/bitcoin/pull/27298
* https://github.com/bitcoin/bitcoin/pull/27363
💬 crediblebytes commented on pull request "doc: revert clarify -datacarriersize":
(https://github.com/bitcoin/bitcoin/pull/29173#issuecomment-1877187993)
> The help text and documentation is not generally a statement of requirements
That is a shame. What the software does and more importantly what the software doesn't do is the very role of software requirements. So first of all get the act together with best software practices. You have a descriptive argument called datacarriersize that doesn't really leave room for debate on what it is supposed to do. It isn't called scriptPubKeySize. However the behavior of this variable was changed withou
...
(https://github.com/bitcoin/bitcoin/pull/29173#issuecomment-1877187993)
> The help text and documentation is not generally a statement of requirements
That is a shame. What the software does and more importantly what the software doesn't do is the very role of software requirements. So first of all get the act together with best software practices. You have a descriptive argument called datacarriersize that doesn't really leave room for debate on what it is supposed to do. It isn't called scriptPubKeySize. However the behavior of this variable was changed withou
...
💬 darosior commented on pull request "fuzz: a target for the block index database":
(https://github.com/bitcoin/bitcoin/pull/28209#discussion_r1441828968)
Heh, no that's a good point. There is a bunch happening in CDBWrapper's constructor so i'm sure this would reduce the time we spend on uninteresting inputs.
(https://github.com/bitcoin/bitcoin/pull/28209#discussion_r1441828968)
Heh, no that's a good point. There is a bunch happening in CDBWrapper's constructor so i'm sure this would reduce the time we spend on uninteresting inputs.
💬 brunoerg commented on pull request "fuzz: a target for the block index database":
(https://github.com/bitcoin/bitcoin/pull/28209#discussion_r1441845813)
Nevermind, my bad. It's better to reuse the same variable.
(https://github.com/bitcoin/bitcoin/pull/28209#discussion_r1441845813)
Nevermind, my bad. It's better to reuse the same variable.
💬 achow101 commented on pull request "sqlite: Disallow writing from multiple `SQLiteBatch`s":
(https://github.com/bitcoin/bitcoin/pull/29112#discussion_r1441855248)
Done
(https://github.com/bitcoin/bitcoin/pull/29112#discussion_r1441855248)
Done
💬 fanquake commented on pull request "Update libsecp256k1 subtree to current master":
(https://github.com/bitcoin/bitcoin/pull/29169#issuecomment-1877229355)
I've updated to commit message to make it clear that this isn't exactly the 0.4.1 tag.
> Plus, ./configure --help in the subtree mentions
I was thinking more like output from bitcoind or similar.
(https://github.com/bitcoin/bitcoin/pull/29169#issuecomment-1877229355)
I've updated to commit message to make it clear that this isn't exactly the 0.4.1 tag.
> Plus, ./configure --help in the subtree mentions
I was thinking more like output from bitcoind or similar.